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

api: Further improvements to the CORS logic #1001

Merged
merged 14 commits into from
May 31, 2022
Merged

api: Further improvements to the CORS logic #1001

merged 14 commits into from
May 31, 2022

Conversation

victorges
Copy link
Member

@victorges victorges commented Apr 12, 2022

What does this pull request do? Explain your changes. (required)

This is a follow up on #985, addressing some additional issues discussed during
the #load-test session today regarding the security of these new changes made
to the CORS handling of the API.

Specific updates (required)

  • Explicitly block "post-flight" requests with a 403 if their API key does not grant the proper CORS access (non-standard behavior, but kind of necessary due to our also non-standard passthrough in the OPTIONS pre-flight)
  • Disallow admins to use CORS API keys. Just an additional security measure since we have a lot of people registered as admins now and we certainly don't want those keys to be in web pages out there.
  • Use the "access rules" internal library for the CORS authorization. This will allow us to add CORS rules to the analyzer (stream health) APIs as well in the future (won't be a drop-in tho, also need the analyzer to start forwarding the CORS headers)
  • Easter egg: hover on your name in the dashboard and you'll see the logged in email

-

  • How did you test each of these updates (required)
  • Ran the server locally and made the appropriate tests.
  • yarn test (I'll work on adding some unit tests for cors)

Does this pull request close any open issues?
Not yet. Fixes some things we talked about in the #load-test.

Screenshots (optional):
Screen Shot 2022-04-12 at 19 04 39

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@victorges victorges requested a review from a team as a code owner April 12, 2022 22:05
@vercel
Copy link

vercel bot commented Apr 12, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/livepeer/livepeer-com/BDYJrG6bW3sJ6zYRjebv487x447W
✅ Preview: https://livepeer-com-git-vg-featcors-v2-livepeer.vercel.app

[Deployment for 75f4a1a failed]

Copy link
Member

@hjpotter92 hjpotter92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vercel
Copy link

vercel bot commented May 30, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-com ✅ Ready (Inspect) Visit Preview May 31, 2022 at 10:11PM (UTC)

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #1001 (b19c4c4) into master (b8670e4) will increase coverage by 0.07479%.
The diff coverage is 59.40594%.

❗ Current head b19c4c4 differs from pull request most recent head 8a2ddb6. Consider uploading reports for the commit 8a2ddb6 to get more accurate results

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1001         +/-   ##
===================================================
+ Coverage   49.22405%   49.29884%   +0.07479%     
===================================================
  Files             66          66                 
  Lines           4124        4136         +12     
  Branches         724         730          +6     
===================================================
+ Hits            2030        2039          +9     
  Misses          1851        1851                 
- Partials         243         246          +3     
Impacted Files Coverage Δ
packages/api/src/controllers/stream.ts 48.40764% <53.24675%> (ø)
packages/api/src/middleware/auth.ts 82.85714% <70.58824%> (-1.18542%) ⬇️
packages/api/src/controllers/api-token.js 56.62651% <100.00000%> (+1.62651%) ⬆️
packages/api/src/controllers/asset.ts 36.32287% <100.00000%> (-0.56603%) ⬇️
packages/api/src/controllers/multistream.ts 79.01235% <100.00000%> (ø)
packages/api/src/controllers/session.ts 15.83333% <100.00000%> (ø)
packages/api/src/controllers/task.ts 13.26531% <100.00000%> (ø)

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 b8670e4...8a2ddb6. Read the comment docs.

@victorges victorges merged commit 15564ae into master May 31, 2022
@victorges victorges deleted the vg/feat/cors-v2 branch May 31, 2022 22:07
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