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

feat: add redirect param #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

luizstacio
Copy link
Member

@luizstacio luizstacio commented Dec 22, 2023

  • Add a redirect=true query param, to redirect the user to the previous page referrer once faucet is complete.
  • Add a documentation section on the README with available query params.

@bvrooman
Copy link
Contributor

bvrooman commented Dec 24, 2023

There are potential security concerns related to unvalidated redirects that are made client side.

If I understand correctly, the flow is something like this:

  1. The user enters dApp URL into the browser and arrives at dApp page
  2. The user needs tokens, so the dApp brings the user to the faucet, storing the dApp URL in the param
  3. The user receives their tokens and the faucet redirects back to the dApp using the given URL param

This is a potential vector for phishing attacks. If the user were to arrive at the faucet through a compromised link, they could be redirected back to a malicious dApp. This could also happen if a user's browser is compromised in some way that allows an attacker to modify URL params, such as a compromised browser extension.

When it comes to redirects, some protocols (e.g., OAuth2) mitigate this by requiring that the URL be registered by the application server side beforehand. They validate a request by comparing the URL param against the registered URL(s), effectively acting as a whitelist. The OWASP link talks more about this and possible mitigations.

@luizstacio
Copy link
Member Author

luizstacio commented Jan 8, 2024

There are potential security concerns related to unvalidated redirects that are made client side.

If I understand correctly, the flow is something like this:

  1. The user enters dApp URL into the browser and arrives at dApp page
  2. The user needs tokens, so the dApp brings the user to the faucet, storing the dApp URL in the param
  3. The user receives their tokens and the faucet redirects back to the dApp using the given URL param

This is a potential vector for phishing attacks. If the user were to arrive at the faucet through a compromised link, they could be redirected back to a malicious dApp. This could also happen if a user's browser is compromised in some way that allows an attacker to modify URL params, such as a compromised browser extension.

When it comes to redirects, some protocols (e.g., OAuth2) mitigate this by requiring that the URL be registered by the application server side beforehand. They validate a request by comparing the URL param against the registered URL(s), effectively acting as a whitelist. The OWASP link talks more about this and possible mitigations.

I remove the redirectUrl param and implement a solution that only redirects the user back from the referrer page, this removes the vector attack of having a shared link with fuel.network directly. But yet is possible to create a redirect page that once clicked redirects to the faucet page in this way the referrer will be the previous page.

The solution I propose on Slack also was to add a hardcoded list inside the repo, this means that any DApp that wants the redirect would need to open a PR, this is the better way to control to who we allow redirects.

cc: @bvrooman @Voxelot

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.

None yet

2 participants