Secure token in POST body or auth header #697

ssylvia · 2020-04-27T20:07:16Z

Adds two option to the IRequestOption:

~~fetchMode: specify the mode to fetch with (cors, no-cors, etc)~~
secureToken: hides the token of GET request in X-Esri-Authorization header.

Both options are disabled by default. This could become the default at a major release.

Fixes: #557

codecov · 2020-04-27T20:12:04Z

Codecov Report

Merging #697 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #697   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          108       108           
  Lines         1637      1644    +7     
  Branches       291       293    +2     
=========================================
+ Hits          1637      1644    +7

Impacted Files	Coverage Δ
packages/arcgis-rest-request/src/request.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2229f7b...bc13405. Read the comment docs.

tomwayson

Thanks @ssylvia

I really like the idea of secureToken as you've designed it.

I'm less sure about fetchMode b/c I'm wary of exposing yet another fetch option via IRequestOptions. What is fetch's default mode? If it's not no-cors then couldn't we do this w/o adding fetchMode? People have the option of passing custom fetch so if they want to specify no-cors mode they could use that, right?

I'd also like @patrickarlt to have eyes on this.

packages/arcgis-rest-request/src/request.ts

tomwayson · 2020-05-01T00:13:35Z

packages/arcgis-rest-request/src/utils/IRequestOptions.ts

@@ -14,6 +15,10 @@ export interface IRequestOptions {
   * The HTTP method to send the request with.
   */
  httpMethod?: HTTPMethods;
+  /**
+   * The fetch mode to send the request with.


Let's add link to some docs on this, maybe https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Syntax

tomwayson · 2020-05-01T00:17:01Z

packages/arcgis-rest-request/src/utils/FetchMode.ts

+/**
+ * Fetch mode used by the ArcGIS REST API.
+ */
+export type FetchMode = "cors" | "no-cors" | "same-origin";


I don't know if we need a type for this. Can we just put the string union inline in IRequestOptions for now?

tomwayson · 2020-05-01T00:22:11Z

packages/arcgis-rest-request/src/request.ts

+      // Mixin headers from request options
+      fetchOptions.headers = {
+        ...options.headers
+      };
+


I'm not sure we should move this above the blocks below. I know why you did, but I think if someone passes in an X-Esri-Authorization as well as secureToken, then that should trump what we generate.

tomwayson · 2020-05-01T00:24:56Z

packages/arcgis-rest-request/test/request.test.ts

+        expect(url).toEqual(
+          "https://www.arcgis.com/sharing/rest/content/items/43a8e51789044d9480a20089a84129ad/data"
+        );
+        expect(options.body).toContain("token=token");


should we also expect the method to be POST?

tomwayson

This is great @ssylvia.

I have a slight preference for my suggestion below, but let me know what you think.

I've asked the others to weigh in on this PR, if I don't hear vocal opposition we'll get this merged and released tomorrow.

tomwayson · 2020-05-03T17:04:37Z

packages/arcgis-rest-request/src/request.ts

+          // Add token back to body with other params instead of header
+          if (token.length && options.secureToken) {
+            params.token = token;
+            delete requestHeaders["X-Esri-Authorization"];
+          }


I think you are right to move the token out of the header and into the body if we have to use POST.

I only wonder if we should check for the existence of the token in the header rather than the re-check the criteria for putting it there.

Suggested change

// Add token back to body with other params instead of header

if (token.length && options.secureToken) {

params.token = token;

delete requestHeaders["X-Esri-Authorization"];

}

const headerToken = requestHeaders["X-Esri-Authorization"];

if (headerToken) {

// Add token back to body with other params instead of header

params.token = headerToken;

delete requestHeaders["X-Esri-Authorization"];

}

@tomwayson We can do that but it will get more complicated. When passed as a header, the value is different. It's actually Bearer ${token}. Thought it would be better to use our original which we know is not modified elsewhere.

let's leave it as is

patrickarlt

This looks good to me. I am a little concerned that secureToken implies that tokens are insecure in other contexts. I'm not entirely sure what to rename it to however.

tomwayson · 2020-05-04T15:31:07Z

Thanks @patrickarlt

Some name ideas:

noTokenInUrl: short, to the point. A little "No Country for Old Men" for my taste
useAuthHeaderForGet: this is a horrible name, but it does what it says
useAuthHeader: less horrible, but in this case it might be a little confusing when we don't for POST requests. This could be a string like 'get' | 'always', but we'd have to change the current logic to honor that.

mpayson · 2020-05-05T00:12:31Z

hideToken?

tomwayson · 2020-05-07T04:31:36Z

Thanks for this contribution and your patience, @ssylvia, it's been released as v2.13.0

aert-nearmap · 2022-02-16T22:03:10Z

I'm curious, why was the choice made to only put the token into the header for a GET and not a POST too (for consistency if anything)?

Secure token in POST body or auth header

59a8133

tomwayson reviewed May 1, 2020

View reviewed changes

ssylvia added 2 commits May 1, 2020 12:46

PR changes - Remove fetchMode option

d71327a

Cleanup comments

058bd55

tomwayson reviewed May 3, 2020

View reviewed changes

patrickarlt approved these changes May 4, 2020

View reviewed changes

ssylvia and others added 2 commits May 5, 2020 09:19

Rename secureToken to hideToken

62fa5b2

Merge branch 'master' into secure-token

bc13405

tomwayson merged commit 8595fab into Esri:master May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secure token in POST body or auth header #697

Secure token in POST body or auth header #697

ssylvia commented Apr 27, 2020 •

edited

Loading

codecov bot commented Apr 27, 2020 •

edited

Loading

tomwayson left a comment

tomwayson May 1, 2020

tomwayson May 1, 2020

tomwayson May 1, 2020

tomwayson May 1, 2020

tomwayson left a comment •

edited

Loading

tomwayson May 3, 2020

ssylvia May 4, 2020

tomwayson May 4, 2020

patrickarlt left a comment

tomwayson commented May 4, 2020

mpayson commented May 5, 2020

tomwayson commented May 7, 2020

aert-nearmap commented Feb 16, 2022

Secure token in POST body or auth header #697

Secure token in POST body or auth header #697

Conversation

ssylvia commented Apr 27, 2020 • edited Loading

codecov bot commented Apr 27, 2020 • edited Loading

Codecov Report

tomwayson left a comment

Choose a reason for hiding this comment

tomwayson May 1, 2020

Choose a reason for hiding this comment

tomwayson May 1, 2020

Choose a reason for hiding this comment

tomwayson May 1, 2020

Choose a reason for hiding this comment

tomwayson May 1, 2020

Choose a reason for hiding this comment

tomwayson left a comment • edited Loading

Choose a reason for hiding this comment

tomwayson May 3, 2020

Choose a reason for hiding this comment

ssylvia May 4, 2020

Choose a reason for hiding this comment

tomwayson May 4, 2020

Choose a reason for hiding this comment

patrickarlt left a comment

Choose a reason for hiding this comment

tomwayson commented May 4, 2020

mpayson commented May 5, 2020

tomwayson commented May 7, 2020

aert-nearmap commented Feb 16, 2022

ssylvia commented Apr 27, 2020 •

edited

Loading

codecov bot commented Apr 27, 2020 •

edited

Loading

tomwayson left a comment •

edited

Loading