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.

Sign up for GitHub

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

Jump to bottom

[ENGG-1689] MV3: Expose onBeforeAjaxRequest hook to forward auth header in cross origin redirects #1664

Merged

wrongsahil merged 27 commits into master from mv3-onBeforeAjaxRequest

May 12, 2024

Contributor

lazyvab commented May 5, 2024

Closes issue:

📜 Summary of changes:

✅ Checklist:

Make sure linting and unit tests pass.
No install/build warnings introduced.
Verified UI in browser.
For UI changes, added/updated analytics events (if applicable).
For changes in extension's code, manually tested in Chrome and Firefox.
For changes in extension's code, both MV2 and MV3 are covered.
Added/updated unit tests for this change.
Raised pull request to update corresponding documentation (if already exists).

🧪 Test instructions:

🔗 Other references:


          MV3: Expose onBeforeAjaxRequest hook for service worker to register n…

bd75bf1

…ew DNR rules

lazyvab requested review from sachinjain024, wrongsahil and nafees87n

May 5, 2024 17:23


          Merge branch 'master' into mv3-onBeforeAjaxRequest

bd161d3

deepsource-io bot commented May 5, 2024 •

edited

Here's the code health analysis summary for commits 027228f..81f6546. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
Shell	✅ Success		View Check ↗
JavaScript	❌ Failure	❗ 24 occurences introduced 🎯 3 occurences resolved	View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

nafees87n reviewed

View reviewed changes

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated Show resolved Hide resolved

nafees87n added 6 commits

May 6, 2024 00:53


          added request processor

5fd9294


          fix: tabService cleanup

f79a111


          fix: cache the rules

6fd3b69


          added todo comment

6a12905


          Merge branch 'master' of github.com:requestly/requestly into mv3-onBe…

5ebdf30

…foreAjaxRequest


          update timeout

7b9e69b

lazyvab commented

View reviewed changes

browser-extension/mv3/src/service-worker/services/rulesManager.ts Outdated Show resolved Hide resolved

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated Show resolved Hide resolved

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated Show resolved Hide resolved

nafees87n added 4 commits

May 7, 2024 00:23


          cleanup AJAX interception

9a5fb07


          cleanup request processor

33b9555


          remove logs

fbd91c2


          fix: excluded

c57153b

nafees87n changed the title ~~MV3: Expose onBeforeAjaxRequest hook for service worker to register new DNR rules~~ [ENGG-1689] MV3: Expose onBeforeAjaxRequest hook to forward auth header in cross origin redirects

nafees87n added 5 commits

May 7, 2024 11:40


          fix: excluded domains

700a21c


          added comment

7cf95cc


          moved to psmh


          refactor: methods

320c482


          remove session rules on extension status toggle

799ed06

wrongsahil requested changes

View reviewed changes

browser-extension/mv3/src/client-scripts/ajaxRequestInterceptor.js Outdated Show resolved Hide resolved

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

Comment on lines 125 to 129

+                  if (Object.values(this.cachedRules).every((rules) => rules.length === 0)) {
+                    return;
+                  }
+                  await this.forwardIgnoredHeadersOnRedirect(tabId, requestDetails);

Member

wrongsahil May 9, 2024

We should move this in a separate method handleRedirectAuthorizationHeader. We can add cases too here only and keep other logic separate from each other

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

Comment on lines 31 to 74

+                private forwardIgnoredHeadersOnRedirect = async (tabId: number, requestDetails: AJAXRequestDetails) => {
+                  if (
+                    !IGNORED_HEADERS_ON_REDIRECT.some(
+                      (header) => requestDetails.requestHeaders[header] || requestDetails.requestHeaders[header.toLowerCase()]
+                    )
+                  ) {
+                    return;
+                  }
+                  const matchedRule = this.findMatchingRule(
+                    [...this.cachedRules.redirectRules, ...this.cachedRules.replaceRules],
+                    requestDetails.url
+                  );
+                  if (!matchedRule) {
+                    return;
+                  }
+                  const ignoredHeaderValues = IGNORED_HEADERS_ON_REDIRECT.map((header) => ({
+                    name: header,
+                    value: requestDetails.requestHeaders[header] || requestDetails.requestHeaders[header.toLowerCase()],
+                  }));
+                  const redirectedUrl = this.getRedirectedUrl(requestDetails.url, matchedRule);
+                  return this.updateRequestSpecificRules(tabId, requestDetails.url, {
+                    action: {
+                      requestHeaders: ignoredHeaderValues.map((header: { name: string; value: string }) => ({
+                        header: header.name,
+                        value: header.value,
+                        operation: chrome.declarativeNetRequest.HeaderOperation.SET,
+                      })),
+                      type: chrome.declarativeNetRequest.RuleActionType.MODIFY_HEADERS,
+                    },
+                    condition: {
+                      // Exact URL is not used because for replace rules, a marker query param is add which changes the URL so exact URL doesn't match
+                      urlFilter: `|${redirectedUrl}`,
+                      resourceTypes: [chrome.declarativeNetRequest.ResourceType.XMLHTTPREQUEST],
+                      tabIds: [tabId],
+                      requestMethods: [requestDetails.method.toLowerCase() as chrome.declarativeNetRequest.RequestMethod],
+                      excludedInitiatorDomains: ["requestly.io", "requestly.com"],
+                    },
+                  });
+                };

Member

wrongsahil May 9, 2024

I think we will need this logic more large number of cases in future. Lets create a folder for this as separate out logics

requestProcessor
- index.ts
- redirectAuthHeader
- initiatorDomainHeader

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

+                private forwardIgnoredHeadersOnRedirect = async (tabId: number, requestDetails: AJAXRequestDetails) => {
+                  if (
+                    !IGNORED_HEADERS_ON_REDIRECT.some(
+                      (header) => requestDetails.requestHeaders[header] || requestDetails.requestHeaders[header.toLowerCase()]

Member

wrongsahil May 9, 2024

Compare both headers in lowercase.
There might be case when we stored header in lowercase in IGNORED_HEADERS_ON_REDIRECT and the actual header is in uppercase. This logic will fail then

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

+                  const ignoredHeaderValues = IGNORED_HEADERS_ON_REDIRECT.map((header) => ({
+                    name: header,
+                    value: requestDetails.requestHeaders[header] || requestDetails.requestHeaders[header.toLowerCase()],

Member

wrongsahil May 9, 2024

Use both lowercase header keys

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

Comment on lines 49 to 52

+                  const ignoredHeaderValues = IGNORED_HEADERS_ON_REDIRECT.map((header) => ({
+                    name: header,
+                    value: requestDetails.requestHeaders[header] || requestDetails.requestHeaders[header.toLowerCase()],
+                  }));

Member

wrongsahil May 9, 2024

We should do this for only existing headers. Right now it generates for header values which are not present in the request

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

Comment on lines 82 to 85

+                  const redirectedUrl = pair.destination || requestUrl.replace(pair.from, pair.to);
+                  if (redirectedUrl === requestUrl) {
+                    return null;
+                  }

Member

wrongsahil May 9, 2024

Will this work in case of wildcard and regex subsitutions?

Contributor

nafees87n May 9, 2024

Substitutions are not supported in case of replace rules. We need to handle only redirect rule

browser-extension/mv3/src/service-worker/services/requestProcessor.ts Outdated

+                  tabService.setData(tabId, TAB_SERVICE_DATA.SESSION_RULES_MAP, {
+                    ...sessionRulesMap,
+                    [requestUrl]: ruleId,

Member

wrongsahil May 9, 2024 •

edited

Shouldn't ruleIds be an array or map of rulesIds because there might be case when initiator_domain and auth case applies on same URL

How about this?

{
    [requestUrl]: {
        auth_header: ruleId,
        initiator_domain: ruleId,
    }
}

nafees87n added 5 commits

May 9, 2024 16:33


          null safe

dae2618


          refactor: request processor

04aab48


          fix: headers match

098938a


          added filters check

570ff34


          fix: request type

f7847bd


          fix: regex substitution

4d9c0b9

nafees87n requested a review from wrongsahil

May 9, 2024 18:36

nafees87n and others added 4 commits

May 10, 2024 14:27


          fix: matchedRule info

9676ceb


          fix: destructure

801be2a


          refractoring

c358b29


          remove circular dependecies

81f6546

wrongsahil approved these changes

View reviewed changes

wrongsahil merged commit 7ff78a0 into master

2 of 3 checks passed

wrongsahil deleted the mv3-onBeforeAjaxRequest branch

May 12, 2024 21:21

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