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

[fix] handle missing server file for an action request #7958

Merged
merged 11 commits into from Dec 7, 2022
Merged

[fix] handle missing server file for an action request #7958

merged 11 commits into from Dec 7, 2022

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Dec 6, 2022

fixes #7965
fixes #7957

  • Returns a 405 response for an action request when no server file exists for a page.
  • Adds a JSON body for the "cross-origin POST" and "no actions exist" error responses.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Dec 6, 2022

🦋 Changeset detected

Latest commit: 8491ba0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@vercel
Copy link

vercel bot commented Dec 6, 2022

@s3812497 is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@dummdidumm
Copy link
Member

Thank you - I'm not sure if this is the right fix though. Looking at the linked issue, that error response should have been JSON instead - the "Accept" header clearly states that. So we should fix this on the server instead. Are there other situations besides the reproducible in the issue where you experienced this bug?

@eltigerchino
Copy link
Member Author

eltigerchino commented Dec 6, 2022

Ah you're right. I wasn't even thinking of the Accept header.

I assumed that the server was returning plain text responses for all errors because it was also failing to parse the 405 error response when no actions exist (this was what I was trying to fix originally). I think it's this line that's causing it?. Should the server return a JSON response body in most if not all cases?

other situations besides the reproducible in the issue where you experienced this bug?

The server returns text/plain when a +page.server.js file exists and there are no actions.
https://stackblitz.com/edit/sveltejs-kit-template-default-oazk46?file=src%2Froutes%2F%2Bpage.svelte

Otherwise, without a +page.server.js file, it returns text/html.
https://stackblitz.com/edit/sveltejs-kit-template-default-siriup

@dummdidumm
Copy link
Member

Interesting - thanks for investigating! The Accept header should be honored in these cases, so in the progressive enhancement case where JSON is expected for the response, it should be sent as such.
The offending code is inside actions.js and runtime/server/page/index.js (line 41, the "is action request but no +page.server.js exists" case is unhandled).

@eltigerchino eltigerchino changed the title [fix] add content-type check to enhance [fix] handle missing server / action for an action request Dec 6, 2022
@eltigerchino eltigerchino changed the title [fix] handle missing server / action for an action request [fix] handle missing server for an action request Dec 6, 2022
@eltigerchino eltigerchino changed the title [fix] handle missing server for an action request [fix] handle missing server file for an action request Dec 6, 2022
@eltigerchino
Copy link
Member Author

Thanks for the guidance @dummdidumm ! I've added handling for a missing server file using the same "405 POST method not allowed. No actions exist for this page" response. Hopefully I've implemented this correctly.

@dummdidumm
Copy link
Member

Thank you! Ok so this brings up another design question: Right now the response is still not JSONified, which it should be, but if we do that, it ideally should honor the App.Error interface. So this should probably use handle_error_and_jsonify and return a JSON response from that (similar to what's happeneing further below in that method).

@eltigerchino
Copy link
Member Author

eltigerchino commented Dec 6, 2022

Yup, looks much better now.

What about the plain text response from the CSRF check?
Should a JSON response also be made available to prevent the JSON parsing error from enhance?

	if (options.csrf.check_origin) {
		const forbidden =
			request.method === 'POST' &&
			request.headers.get('origin') !== url.origin &&
			is_form_content_type(request);

		if (forbidden) {
			const csrf_error = error(403, `Cross-site ${request.method} form submissions are forbidden`);
			// handle accept: application/json
			if (request.headers.get('accept') === 'application/json') {
				return json(csrf_error.body, { status: csrf_error.status });
			}
			return new Response(csrf_error.body.message, { status: csrf_error.status });
		}
	}

@dummdidumm
Copy link
Member

yeah that makes sense 👍

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit d23ce80 into sveltejs:master Dec 7, 2022
@github-actions github-actions bot mentioned this pull request Dec 7, 2022
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.

Unhandled form action when +page.server does not exist form enhance incorrectly parses response text as JSON
2 participants