-
Notifications
You must be signed in to change notification settings - Fork 2
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: explicitly use URLSearchParams when constructing send to email magic links #4138
Conversation
Removed vultr server and associated DNS entries |
email: teamSettings.submissionEmail, | ||
localAuthority: localAuthority, | ||
}); | ||
const applicationFilesDownloadLink = `${process.env.API_URL_EXT}/download-application-files/${sessionId}?${params}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const applicationFilesDownloadLink = `${process.env.API_URL_EXT}/download-application-files/${sessionId}?${params}`; | |
const applicationFilesDownloadLink = `${process.env.API_URL_EXT}/download-application-files/${sessionId}?${params.toString()}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested yet (doing so now) but this flagged as an issue with this approach!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ignore this sorry - when templated this parses to a string natively without .toString()
- neat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly 🙂 there's a few inconsistent uses of this around the code base we should cleanup sometime though outside of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working for me when testing.
nit: Only thing I would think about adding for robustness is something in the downloadApplicationFiles
code to remove these ;amp
additions. We can't manage how IT teams wrap ext network requests, and from my view it's likely this could happen again so could be good to start a function now to clean the query params?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an issue with Tewksbury's IT wrapping and incorrectly decoding the URL.
The URL is correct and well formatted - nobody else has reported this and using web APIs (URLSearchParams
) is a robust and reliable approach.
We could certainly make this a little bit more robust by -
- constructing a URL using
new URL()
- validating this URL
However - I don't think this would actually resolve the issue here, just give us a better leg to stand on / more confidence on our side of the fence.
BTW - just to be explicit - tested on pizza and working as expected ✅ |
See #help-issues thread from Tewkesbury here: https://opendigitalplanning.slack.com/archives/C0241GWFG4B/p1736501911947289
Here's what's happening:
email_applications
audit table downloads as expectederror: "Missing values required to access application files"
(email forwarded todevops@
)https://linkprotect.cudasvc.com/url?a={our encoded URL}
which fails to properly decode the second query param in the requestamp;localAuthority
when inspecting the network request here:Open questions:
Changes & testing:
new URLSearchParams()
when constructing our magic link may fix this instead of the previous plain string template approach? Any other more robust ideas here?linkprotect
URL, which likely confirms this is unqiue to Tewkesbury and not Gov Notify as a whole