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

Support autoredirection #54

Closed
wants to merge 7 commits into from
Closed

Support autoredirection #54

wants to merge 7 commits into from

Conversation

Jeff-SearchPilot
Copy link
Contributor

This allows the user to pass in the arn as a url argument and be immediately redirected to it.

This allows the user to pass in the arn as a url argument and be immediately redirected to it.
@fxkr
Copy link
Contributor

fxkr commented Mar 20, 2024

Thank you for the contribution. This is an interesting idea.

I apologize in advance that this will take me some time to review. I will need to carefully think about what input validation link2aws is doing and needs to do before we merge this, as this can have security implications (Open Redirect Abuse: link/link).

A few more review thoughts:

  • Privacy: this uses query parameters (?arn=...), which will leak to the server (GitHub, so maybe not really an actual issue). For privacy reasons, it might be nice to use fragments instead (#...), which aren't sent to the server. But we can support both, and for this PR, ?arn= would be fine.

  • Browser compatibility: I wasn't sure, but URLSearchParams seems to be widely supported. So no problem.

  • Also relevant: Investigate search bar integration #8

@Jeff-SearchPilot
Copy link
Contributor Author

Any further thoughts on this @fxkr?

@fxkr
Copy link
Contributor

fxkr commented Apr 3, 2024

Hi @Jeff-SearchPilot , sorry for the delay. I have been traveling and haven't had any time to look at this yet.

Copy link
Contributor

@fxkr fxkr left a comment

Choose a reason for hiding this comment

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

Hi @Jeff-SearchPilot , I have now merged a commit with extra input validation, so that we can go ahead with this now. Can you address the following review comments?

Thank you very much!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
fxkr added a commit that referenced this pull request Apr 9, 2024
@fxkr
Copy link
Contributor

fxkr commented Apr 9, 2024

Thanks! Merged.

@fxkr fxkr closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants