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

Token is exposed in GET request #1364

Closed
acass91 opened this issue Jul 5, 2023 · 5 comments
Closed

Token is exposed in GET request #1364

acass91 opened this issue Jul 5, 2023 · 5 comments

Comments

@acass91
Copy link

acass91 commented Jul 5, 2023

Describe the bug

When connecting to a map service that requires a token, creating a layer exposes the token, which is a sensitive parameter, as part of a GET request. Is there any way to make it a POST request instead?

Reproduction

To reproduce this issue, you will need a service that requires the use of a token. Open your preferred web browser and inspect the network traffic. You should see a GET request being made to the service which includes the token in the url.

Here's the code I use to create a dynamic map layer. I cannot disclose the value of baseUrl here.

L.esri.dynamicMapLayer({
            url: `${baseUrl}/server/rest/services/CC_SNET_Boundary/CC_SNET_HUB_BNDY_DYN/MapServer`,
            opacity: 1,
            useCors: true,
            token: token
        });

Logs

No response

System Info

Leaflet version: 1.0.1 
Esri Leaflet version: 2.2.3

Additional Information

No response

@gavinr-maps
Copy link
Contributor

gavinr-maps commented Jul 21, 2023

Hi @acass91, thank you for logging this issue.

Could you please give us some more information about your use case - why having this in the URL param is an issue for you? Note that HTTPS/SSL does secure GET URL params, but we understand that there are other reasons and situations that may be an issue for this, so we wanted to hear more about your particular case.

That all being said, I discussed this with @patrickarlt and we think the lower-level request module could be updated to support HTTP Header Auth, which should resolve your issue, but it might be a big change so may not have time right now. We would probably accept a PR to make HTTP Header Auth an option though if you'd like to take a crack at it!

Thanks!

@acass91
Copy link
Author

acass91 commented Jul 21, 2023

Hi @gr-maps, my organization reported this to my team as a vulnerability from OWASP.

Truth be told, this isn't really an issue for me, but I am trying to justify not updating our application.

@gavinr-maps
Copy link
Contributor

Ok, thank you for that context. In that case I think this is still applicable:

That all being said, I discussed this with @patrickarlt and we think the lower-level request module could be updated to support HTTP Header Auth, which should resolve your issue, but it might be a big change so may not have time right now. We would probably accept a PR to make HTTP Header Auth an option though if you'd like to take a crack at it!

Thanks!

@BrunoCaimar
Copy link
Contributor

I believe that this PR #1378 can fix this issue. I've tested using it with a FeatureLayer and a DynamicLayer.

This PR solution is similar to the approach that ArcGIS REST JS is using.

@patrickarlt
Copy link
Contributor

Closing this. See my comment in #1378 (comment).

I will add here that there are reasons we do support this in ArcGIS REST JS but not in Esri Leaflet:

  1. ArcGIS REST JS can also be used outside a browser context which can hide the token in GET requests without the complexities of browser preflighting and CORS restrictions.
  2. ArcGIS REST JS generally isn't used to make the kinds of high volume high performance queries that need specific caching requirements like loading feature layers in viral applications that must use GET for caching reasons and can't have a performance hit for preflighting requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants