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.

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

Fix/proxy cors #215

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Fix/proxy cors #215

merged 3 commits into from
Aug 10, 2023

Conversation

bdmendes
Copy link
Collaborator

Closes #158

Review checklist

  • Contains enough appropriate tests
  • Behavior is as expected
  • Clean, well-structured code

@codecov
Copy link

codecov bot commented Jul 14, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (fa4e2be) 95.38% compared to head (4e5ffdc) 95.38%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #215   +/-   ##
========================================
  Coverage    95.38%   95.38%           
========================================
  Files           16       16           
  Lines          130      130           
  Branches        18       18           
========================================
  Hits           124      124           
  Misses           6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

limwa
limwa approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@limwa limwa left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
Left a comment, but it's just a question and not really a request for changes!

src/routes/api/[...endpoint]/+server.ts
@@ -11,3 +11,6 @@ export const GET: RequestHandler = dispatchToBackend;
export const POST: RequestHandler = dispatchToBackend;
export const PUT: RequestHandler = dispatchToBackend;
export const DELETE: RequestHandler = dispatchToBackend;
export const PATCH: RequestHandler = dispatchToBackend;
export const OPTIONS: RequestHandler = dispatchToBackend;
export const HEAD: RequestHandler = dispatchToBackend;
Copy link
Member

Choose a reason for hiding this comment

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

Is adding CONNECT and TRACE as well worth it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do not think so, they are not the type of requests we would do to the server via the frontend

@limwa
Copy link
Member

limwa commented Jul 19, 2023

Also, I know that it is out of the scope of this PR but won't this line (https://github.com/NIAEFEUP/website-niaefeup-frontend/pull/215/files#diff-4bcda89884a979801f8a542290893a21e5213fc48218ec305b5434ef11e70ddaR6) cause an error when the request body is, for instance, an image?

limwa
limwa requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@limwa limwa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I just noticed that HEAD and OPTIONS requests don't have a body. Line 6 of +server.ts should be changed accordingly. I also left another suggestion so let me know what you think.

src/routes/api/[...endpoint]/proxy.ts
Comment on lines +15 to +17
if (window?.location?.origin) {
headers.append('Origin', window.location.origin);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the Origin header is automatically added by browsers when fetch is called, so it shouldn't be necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is, it's just a way to make sure it is there. I'm not sure how could we have a cors problem otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I think the cors problem was because OPTIONS requests (used by CORS) weren't being proxied to the backend. I'll approve either way.

@bdmendes
Copy link
Collaborator Author

Also, I know that it is out of the scope of this PR but won't this line (https://github.com/NIAEFEUP/website-niaefeup-frontend/pull/215/files#diff-4bcda89884a979801f8a542290893a21e5213fc48218ec305b5434ef11e70ddaR6) cause an error when the request body is, for instance, an image?

I am not sure that it would cause an error (did a quick search and the docs do not appear to mention that), maybe it will simply be a utf-8 string representation of the bytes.
I do agree that that may not be the best way to handle those cases, feel free to open an issue about that.

limwa
limwa approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@limwa limwa left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@limwa
Copy link
Member

limwa commented Jul 19, 2023

Also, I know that it is out of the scope of this PR but won't this line (https://github.com/NIAEFEUP/website-niaefeup-frontend/pull/215/files#diff-4bcda89884a979801f8a542290893a21e5213fc48218ec305b5434ef11e70ddaR6) cause an error when the request body is, for instance, an image?

I am not sure that it would cause an error (did a quick search and the docs do not appear to mention that), maybe it will simply be a utf-8 string representation of the bytes. I do agree that that may not be the best way to handle those cases, feel free to open an issue about that.

From my testing, you're right, it doesn't error. However, it still means we won't be able to upload images (or other binary formats) properly to our backend. I'll open the issue.

@limwa limwa mentioned this pull request Jul 20, 2023
Open
@BrunoRosendo
Copy link
Member

I can't test because of this error:

Error: Invalid export 'HEAD' in src/routes/api/[...endpoint]/+server.ts (valid exports are GET, POST, PATCH, PUT, DELETE, OPTIONS, prerender, trailingSlash, config, or anything with a '_' prefix)

Did you encounter it?

@limwa
Copy link
Member

limwa commented Jul 26, 2023

I can't test because of this error:

Error: Invalid export 'HEAD' in src/routes/api/[...endpoint]/+server.ts (valid exports are GET, POST, PATCH, PUT, DELETE, OPTIONS, prerender, trailingSlash, config, or anything with a '_' prefix)

Did you encounter it?

That issue has been fixed in SvelteKit v1.22.0 (https://github.com/sveltejs/kit/blob/master/packages/kit/CHANGELOG.md).

I just noticed that we are using the dependency "@sveltejs/kit" with the tag "next". As per this page, that will resolve to version "1.0.0-next.589". We might want to change the tag in the package.json file to "latest" or something else as that version is very outdated (7 months old).

@limwa limwa mentioned this pull request Aug 9, 2023
Merged
3 tasks
BrunoRosendo
BrunoRosendo approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@BrunoRosendo BrunoRosendo left a comment

Choose a reason for hiding this comment

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

lgtm, the issue should be solved after bumping sveltekit version

@BrunoRosendo BrunoRosendo merged commit a176aab into develop Aug 10, 2023
@BrunoRosendo BrunoRosendo deleted the fix/proxy-cors branch August 10, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrunoRosendo BrunoRosendo BrunoRosendo approved these changes

@limwa limwa limwa approved these changes

@MRita443 MRita443 Awaiting requested review from MRita443

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

api: Fix proxy not using CORS for backend requests
3 participants
@bdmendes @limwa @BrunoRosendo