search

ankit / stylebot

Change the appearance of the web instantly
https://stylebot.dev/
MIT License
1.38k stars 205 forks source link

Fix matchesBasicPattern sometimes matching on incorrect domains #690

Closed lionel-rowe closed 2 years ago

lionel-rowe commented 2 years ago

Fixes #685: Default domain matching sometimes matches on incorrect domains.

See src/background/__tests__/utils.test.ts for details of the new matching logic.

strobelight commented 2 years ago

looking forward to this, but in my case, the domain looks the same but isn't being matched, so not sure if a UTF thing or what.

lionel-rowe commented 2 years ago

looking forward to this, but in my case, the domain looks the same but isn't being matched, so not sure if a UTF thing or what.

What's the pattern and the URL it should be matching? Is there an open GitHub issue for this? If it's relevant and should indeed match, I can add a test for it in the PR (no idea when/if this is gonna get merged though).

strobelight commented 2 years ago

What's the pattern and the URL it should be matching? Is there an open GitHub issue for this? If it's relevant and should indeed match, I can add a test for it in the PR (no idea when/if this is gonna get merged though).

It's an internal url to a wiki, and oddly doesn't work on only one page where the div id's match. Tried to build the patch but it failed, but then again, I have no experience with yarn, npm, and the readme's don't really provide details for the uninitiated. I cloned stlyebot-fix-685, which couldn't apply some patch. So, hence I'm looking forward to this being merged in to give it a shot.

lionel-rowe commented 2 years ago

It's an internal url to a wiki, and oddly doesn't work on only one page where the div id's match. Tried to build the patch but it failed, but then again, I have no experience with yarn, npm, and the readme's don't really provide details for the uninitiated. I cloned stlyebot-fix-685, which couldn't apply some patch. So, hence I'm looking forward to this being merged in to give it a shot.

The URL being internal won't matter, what matters is its structure. Can you post it and the pattern here? If there's sensitive info in there, you can always replace it with foo/bar etc.

Also, are you certain it's a URL matching issue rather than an element matching issue? If you add a style rule of * { outline: 3px solid red; }, does that also fail to show up on the affected page?

strobelight commented 2 years ago

The URL being internal won't matter, what matters is its structure. Can you post it and the pattern here? If there's sensitive info in there, you can always replace it with foo/bar etc.

Something like: https://etwiki.sys.company.net/display/~mememe/x.json

Pattern: etwiki.sys.company.net

Also, are you certain it's a URL matching issue rather than an element matching issue? If you add a style rule of * { outline: 3px solid red; }, does that also fail to show up on the affected page?

At the above url, the red outlines did not show.

If I change the name of the wiki from x.json to xjson, all worked well, red outlines appeared.

But again, that's with the current version, not yours. So if your changes keep the host matching to the first slash after the host, it should be better.

Seems to be related to the .json ending since a x.json file also works. Also tried x.tar, x.mp4, x.tgz which also worked, so the issue with current version ignored .json.

Wish I could try your version.

lionel-rowe commented 2 years ago

Something like: https://etwiki.sys.company.net/display/~mememe/x.json

Pattern: etwiki.sys.company.net

That URL matches with both the old and the new logic, but judging by the extension, it's a JSON file, and CSS is for styling HTML rather than JSON. Why d'you want to style a JSON file anyway? JSON is a data interchange format, rather than a format for displaying to end users. Or is it actually just an HTML file that misleadingly ends in .json?

strobelight commented 2 years ago

That URL matches with both the old and the new logic, but judging by the extension, it's a JSON file, and CSS is for styling HTML rather than JSON.

well, it's really a wiki that happens to be named "x.json", and so the browser I guess is confused, or the confluence server really did set the content type as json.

I got around it by renaming the page, but to me, it shouldn't matter what the name is.

strobelight commented 2 years ago

I checked x.json wiki again, and in debugger the content type is: content-type: text/html;charset=UTF-8 for what it's worth.

lionel-rowe commented 2 years ago

well, it's really a wiki that happens to be named "x.json"

Ahh I see. This PR has no effect on how content types are determined, so you'll need to open a separate issue.