Cors with credentials fix

[justus237]

Fixes #933 by extending the respondUnallowedMethod function and setting CORS headers differently in case there is a security scheme other than NoSec.

[justus237]

Guess the ECA thing clashes with GitHub's email privacy...

[danielpeintner]

Guess the ECA thing clashes with GitHub's email privacy...

I restarted ECA validation and it is fine now 👍

I will take a look at the code and test it soon.
I think @relu91 should take a look as well...

[justus237]

The latest commit causes OAuth tests to time out, will have to look into why. Can probably cancel the current CI run.

[justus237]

@danielpeintner see https://github.com/justus237/thingweb.node-wot/actions/runs/4386764993/jobs/7681232195

[justus237]

I think this should fix it, I think it might make sense to also implement checking for Access-Control-Allow-Headers and mirroring it, instead of the current hard-coded value.

[danielpeintner]

Hi @justus237
How did you test the functionality?

I tried the following. I used https://github.com/justus237/node-wot-cors which fails as is (as expected). Hence I removed package.json and package-lock.json and installed the dependency based on your PR as follows.

npm install https://github.com/eclipse/thingweb.node-wot.git#pull/934/head

Unfortunately I still see the same CORS warnings on the client.

BTW, the delay before I see the CORS issue in the console or the failure alert is still there.

[relu91]

The code logic is a bit convoluted but given the current structure of the http-server, I still think we can merge this. I would be ok with merging this if the underlying issue is resolved (see @danielpeintner comments above).

[danielpeintner]

@relu91 just somehow related to this PR. Does it make sense to collect such "manual" testcases such as https://github.com/justus237/node-wot-cors in the node-wot repo somewhere? What do you and others think?

[relu91]

@relu91 just somehow related to this PR. Does it make sense to collect such "manual" testcases such as https://github.com/justus237/node-wot-cors in the node-wot repo somewhere? What do you and others think?

for starting, I think it would be sufficient to have unit tests and verify the behavior. It is not like testing it in a real browser but once we got the behavior right it should help us to keep it correct.

[justus237]

@danielpeintner When testing, I installed only the HTTP bindings from my (local) fork, i.e., npm install ../thingweb.node-wot/packages/binding-http. Not sure how this differs from your approach, I'm not an expert on npm (or node dependency management in general). I will try testing again tomorrow, but my method worked when I tried it.

[justus237]

@danielpeintner I updated https://github.com/justus237/node-wot-cors with a postinstall hook, so when in a newly cloned version of the repository, running npm install should set everything up correctly.

[danielpeintner]

Thanks @justus237 for your work.
The postinstall script fails on my machine with "The syntax of the command is incorrect." Anyhow, I copied the commands to the shell and everything gets checked-out & installed as expected.

Anyhow, running node server.js and using the HTML on the same machine works but still fails using another box causing CORS issues.
I don't know why in your case it works. Maybe some other problem in my setup.

@relu91 can you give it a try with 2 different machines? Thanks!
If it works in your setup I am fine with preceding...

[relu91]

Yes I'll try tomorrow 👍🏻

[justus237]

Anyhow, running node server.js and using the HTML on the same machine works but still fails using another box causing CORS issues. I 8don't know why in your case it works. Maybe some other problem in my setup.

My testing is done with (link-local) IP addresses and with mDNS domain names. Is your setup on the Internet? Maybe my fix only works in local subnets for some reason.

[danielpeintner]

I was just using local IP addresses a la 192.168.178.46:8082/some-thing
I don't think this should matter, should it?

Anyhow, let's wait for @relu91 to check also.

[relu91]

I've just tried with an HTTP tunnel to my local host. I didn't follow through with the discussion, is it enough? basically, I used ngrok to tunnel the server to a web address (e.g., http://e5b2-146-241-71-170.ngrok.io/) and I also changed the hrefs in forms to http://e5b2-146-241-71-170.ngrok.io/some-thing/properties/status).

If the setup is still valid I can confirm that I didn't see any issue with CORS.

[justus237]

I don't think this should matter, should it?

Yeah, it shouldn’t.

I've just tried with an HTTP tunnel to my local host. I didn't follow through with the discussion, is it enough? basically, I used ngrok to tunnel the server to a web address (e.g., http://e5b2-146-241-71-170.ngrok.io/) and I also changed the hrefs in forms to http://e5b2-146-241-71-170.ngrok.io/some-thing/properties/status).

Just to make sure, if you expose the server without the fix (e.g., with the old package.json) through a tunnel and open the index.html website locally or as file://, the client on the website fails?

[relu91]

Just to make sure, if you expose the server without the fix (e.g., with the old package.json) through a tunnel and open the index.html website locally or as file://, the client on the website fails?

Yup, it fails. With this, I think we can merge this. @danielpeintner are you ok?

[relu91]

We can add unit tests later, just create an issue to keep this in mind.

[danielpeintner]

We can add unit tests later, just create an issue to keep this in mind.

Go ahead 👍