Repository navigation
Conversation
🦋 Changeset detectedLatest commit: c584ebc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit c584ebc
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #598 +/- ##
============================================
- Coverage 56.72% 27.05% -29.67%
============================================
Files 105 11 -94
Lines 28457 1578 -26879
Branches 1644 49 -1595
============================================
- Hits 16143 427 -15716
+ Misses 12314 1151 -11163 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| * flows, not the generic resource proxy | ||
| */ | ||
| if (requestOrigin === amOrigin) { | ||
| // If the request origin was an AM origin and no allowed AM endpoint was targeted then error |
There was a problem hiding this comment.
Can we make this code comment like the others to draw attention to this?
/** ****************************************************
* UNSUPPORTED AM ENDPOINT ERROR
* If the request origin was an AM origin but endpoint
* was unrecognized return error.
*/
| if (!tokens) { | ||
| const errorResponse = createErrorResponse( | ||
| 'no_tokens', | ||
| new Error('No OAuth/OIDC tokens found'), | ||
| ); | ||
| responseChannel.postMessage(errorResponse); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can you share why you added this?
There was a problem hiding this comment.
Previously this check was done right before proxying allowed AM requests that needed a token. That token check is still done but is now wrapped up inside of the parent if (requestOrigin === amOrigin) conditional. So the check is no longer applied before making a fall-through proxied resource request. We need to do the same token check now when we reach this fallback because the request requires the token be sent in the auth header.
There was a problem hiding this comment.
On closer inspection, this is needed to check both tokens.tokenExpiry and tokens.refreshToken downstream. It seems at this point we are still expecting tokens to exist so I think this is valid but lmk otherwise.
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
I don't have anything to add Justin commented on the one block i was looking at as well
This branch has not been deployed
4 participants
JIRA Ticket
Description
Ensure that requests to AM for supported endpoints are only proxied when the request origin is an AM origin. Requests to the resource server with endpoints similar in name to supported AM endpoints should still be allowed with the access token sent only in the authorization header.