Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stricter url matching #335

Merged
merged 2 commits into from
Apr 26, 2022
Merged

fix: stricter url matching #335

merged 2 commits into from
Apr 26, 2022

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented Apr 23, 2022

In user-scripts, you can use the match directive multiple times.
So we match /channels/* since that's what we need for the web app anyway.
We also match /app and /login for when a user is being redirected to the web app from them, totherwise the user script won't be initialized.

This prevents issues like the following on other parts of the website:

/robots.txt

image

The box appearing at the bottom is because of this user-script.

Main Site (i.e. /company)

image

The extra whitespace appearing at the bottom is because of this user-script.

I believe the only pages we'd want the button to appear on would be under /channels anyway, for example:

We then need to cover these because of web app navigation:

I tested the password reset flow (https://discord.com/reset) and can confirm there's no need to match this. It redirects to /app which is already covered.
There's no need to cover /channels itself because that's a 404.

@SethFalco
Copy link
Contributor Author

@abbydiode This might be a good one for you as well?

@victornpb
Copy link
Owner

@SethFalco thank you for the detailed description

@victornpb victornpb added the PR looks good Looks like it's good for merging (maybe be in the next release) label Apr 26, 2022
@victornpb
Copy link
Owner

following the same logic I added ptb and canary urls, someone requested it in another issue

// @match         https://discord.com/app
// @match         https://discord.com/channels/*
// @match         https://discord.com/login
// @match         https://ptb.discord.com/app
// @match         https://ptb.discord.com/channels/*
// @match         https://ptb.discord.com/login
// @match         https://canary.discord.com/app
// @match         https://canary.discord.com/channels/*
// @match         https://canary.discord.com/login

@victornpb victornpb merged commit ed567fb into victornpb:master Apr 26, 2022
@@ -6,6 +6,12 @@
// @match https://discord.com/app
// @match https://discord.com/channels/*
// @match https://discord.com/login
// @match https://ptb.discord.com/app
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// @match https://ptb.discord.com/app
// @match https://*.discord.com/app

Maybe better to cover ptb and canary together like this?
Not sure if that would include no subdomain as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matches any URL that uses the https scheme, is on a google.com host (such as www.google.com, docs.google.com, or google.com), as long as the path starts with /foo and ends with bar

https://developer.chrome.com/docs/extensions/mv3/match_patterns/

Looks like *.discord.com would actually match all 3, the base domain, and both subdomains.

Might be worth replacing the 9 entries with just 3 using *.discord.com. (Haven't tested it manually.)

victornpb added a commit that referenced this pull request Apr 29, 2022
Bump Discord API version to version 9 and add more message type to be deleted  PR looks good (#223 by VictorienXP)
Add wildcard subdomain for discord.com match on the userscript (#224 by VictorienXP)
Fix bug where process is prematurely stopped due to receiving a full array of skippedMessages (#323 by aijorgenson)
fix: stricter url matching (#335 by SethFalco)
refactor: reduce number of @matchs (#336 by SethFalco)
Removed duplicate OR operator (#338 by aydinyal)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR looks good Looks like it's good for merging (maybe be in the next release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants