Allow multiple trusted CORS hosts instead of only one#95
Allow multiple trusted CORS hosts instead of only one#95iekilinc wants to merge 11 commits intoKamWithK:masterfrom
Conversation
This reverts commit eac80c0. It's already ensured at this point that it's not empty.
KamWithK
left a comment
KamWithK
left a comment
There was a problem hiding this comment.
I was on the edge about whether we need this but I've decided it has its uses
app/src/main/java/com/kamwithk/ankiconnectandroid/routing/RouteHandler.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/routing/RouteHandler.java
Outdated
Show resolved
Hide resolved
| rep.addHeader("Access-Control-Allow-Origin", corsHost); | ||
| // Check if the origin matches any of the allowed hosts | ||
| if (normalizedAllowedHosts.contains(normalizedOrigin)) { | ||
| rep.addHeader("Access-Control-Allow-Origin", origin); |
There was a problem hiding this comment.
Does it matter here whether the origin or normalised origin is used here?
There was a problem hiding this comment.
I'm sending out the origin as it came in the request so that there's no chance of our normalization messing up what comes out. The point of the normalized string in my head is just for internally checking if the URLs are the same (though not going through the effort of fully parsing and then reconstruction the URLs as the only thing I'm normalizing for is leading/tailing whitespace and a trailing slash), not to effect the output of the origin.
There was a problem hiding this comment.
I feel like just remove this everywhere and double check everything still works is the go, but I do understand why you did it
There was a problem hiding this comment.
If you are going to do it then the normalised versions should've been used everywhere
There was a problem hiding this comment.
It's unnecessary, so I won't normalize the origin we get from the library.
app/src/main/java/com/kamwithk/ankiconnectandroid/routing/RouteHandler.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/routing/RouteHandler.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/SettingsActivity.java
Outdated
Show resolved
Hide resolved
This reverts commit a322313.
| if (normalizedAllowedHosts.contains("*")) { | ||
| // Since "*" is in the allowed hosts, simply allow all origins | ||
| applyHeaders(rep, "*"); | ||
| } else if (normalizedAllowedHosts.contains(normalizeHost(origin))) { |
There was a problem hiding this comment.
You comment about whether or not normalizing the host we get from the NanoHTTPD's request headers matters now applies to here. The thing is, the library gives us a nice, clean string without the need to trim and without the need to remove the trailing slash. It just feels right to compare a manually-normalized string with a string that was normalized by the same function, but it's really not necessary, so I can remove it if you want.
There was a problem hiding this comment.
If it definitely isn't needed then please remove it
| app:dialogTitle="Enter CORS Host" | ||
| app:dialogMessage="Enter the hostnames you want to allow access from. Each hostname should be on a new line." | ||
| app:dialogTitle="Enter CORS Hosts" | ||
| app:key="cors_host" |
There was a problem hiding this comment.
You may have an issue with the preference being called cors_host instead of cors_hosts internally.
I left it as is to not have to write a migration from the old pref to the new one in order to not break existing users.
There was a problem hiding this comment.
Fair enough
Most people probably don't use this though anyways? Given it could only hold one option in the past but we are opening it up now to be able to accept multiple I think getting people to revisit the option is fine
There was a problem hiding this comment.
Is your final verdict to rename it, thus forcing users to go and redo the setting? That could cause some initial confusion, but fair enough.
Or do you want to leave it as is?
3 participants
Currently, the user can only have one trusted host. My use case is that I want to add, for example, Mokuro Reader, asbplayer, and Mangatan to the CORS hosts instead of only one of them. Right now, the user is forced to either enable all with
"*", which is insecure, or they're forced to switch betweenhttps://app.mokuro.reader,https://app.asbplayer.dev, andhttp://wherever-theyre-hosting-mangatan:4568, which is quite inconvenient.Also, the example text that should show up for the CORS setting was not being displayed due to a mismatch of strings, so that's also fixed.