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

[10.x] Turn off autocomplete for csrf_field #48371

Merged
merged 2 commits into from Sep 14, 2023
Merged

Conversation

maxheckel
Copy link
Contributor

@maxheckel maxheckel commented Sep 12, 2023

Fixes issue #48370.

Autocomplete will fill in the _token field on mobile safari.

@maxheckel maxheckel changed the title Turn off autocomplete Turn off autocomplete for csrf_field Sep 12, 2023
@milwad-dev
Copy link
Contributor

It is a hidden input and there is no need to autocomplete!

@valorin
Copy link
Contributor

valorin commented Sep 12, 2023

This seems like a browser bug, not a bug in Laravel.

The browser should only be autocompleting fields which are marked as autocomplete in the first place, and it really shouldn't be autocompleting a hidden input.

It sounds more like Safari didn't refresh the page when you went to it and you still had an older CSRF token.
Can you replicate the issue on demand with a fresh tab/window?

@maxheckel
Copy link
Contributor Author

Yes, it happens every time I log in on mobile safari, even when navigating to it directly from a new page. If I manually typed my username and password it would not throw the 419 error.

@taylorotwell
Copy link
Member

I mean - I agree with others I don't see how this would possibly be necessary, but also wouldn't break anything I guess. It seems like we would have more reports of this if it were truly an issue?

@valorin
Copy link
Contributor

valorin commented Sep 13, 2023

Safari is the IE6 of the modern internet, so anything is possible...

If the password manager has somehow mistakenly stored the CSRF token, then I can see how it might happen. Not sure why it would have done that originally though...
Can you view the password record? Does it have the CSRF field listed?

But yeah, it shouldn't affect anything by being merged, and if it's proven to work, then it's probably useful to have.

It's worth pointing out that many browsers ignore autocomplete=off, but if this has been tested locally as a working fix, then that's not an issue in this case.

@donnysim
Copy link
Contributor

From my experience even autocomplete is not really respected by password managers, but never seen a readonly input be saved. Maybe have both?

@taylorotwell taylorotwell merged commit b6f4732 into laravel:10.x Sep 14, 2023
19 checks passed
@rodrigopedra
Copy link
Contributor

I always add this snippet on my projects to prevent 419 on Safari from autofilling old input

// prevent ios safari stale page from cache
window.onpageshow = function (event) {
    if (event.persisted) window.location.reload();
};

I never even imagined it was due to the CRSF field being autofilled... Even if I could debug it (no mac here), I guess I wouldn't look for this.

But the 419 on trying to log in from a cached page, such when one adds a page as a home shortcut, was very annoying, and I ended finding the snippet above.

Thanks, @maxheckel

@maxheckel
Copy link
Contributor Author

such when one adds a page as a home shortcut

This was an auto-suggested link on safari, so I'm guessing that behaves in the same way as a home shortcut? Thanks for merging!

@cotiga
Copy link

cotiga commented Nov 19, 2023

Mr. Taylor, the problem is that we make the Web and some people want to respect the W3C!

Better something like this :

return new HtmlString('<input type="text" style="display:none" name="_token" value="'.csrf_token().'">');

@rodrigopedra
Copy link
Contributor

rodrigopedra commented Nov 19, 2023

@cotiga can you point which W3C document does this disrespect? Maybe you mean WHATWG?

Either way, you should consider asking the browser's maintainers to comply with whatever behavior you think it should comply.

One of the ideas of using a framework, is to ease these low-level differences between vendors. So we, as developers, can focus on bringing value to our users.

From a framework perspective, I'd rather benefit end-users, than to comply with some spec a particular browser vendor does not implement.

What do you suggest doing when an end-user has CSS disabled? Or uses a custom CSS, due to any accessibility needs they could have?

The proposed solution is insufficient in that case.

Any way, if you value spec conformance that much over end-user experience, and don't care about users who have custom CSS needs, Laravel allows you to define a custom directive.

https://laravel.com/docs/10.x/blade#extending-blade

It should be as simple as adding this to a Service Provider's boot method:

Blade::directive('whatwgCompliantCSRFToken', function () {
    return '<input type="text" style="display:none" name="_token" value="<?php echo csrf_token(); ?>">';
});

Then, on your blade views, you can use it like this:

<form>
@whatwgCompliantCSRFToken
</form>

That should make the validator happy.

@valorin
Copy link
Contributor

valorin commented Nov 25, 2023

@maxheckel I have memories of a comment somewhere, where you confirmed the issue was Safari caching the page and not an autocomplete issue with the hidden CSRF field.

Are you able to confirm the issue was Safari caching a page and not autocomplete on the CSRF field?

This fix is causing HTML violation reports and PRs with inline styles in attempts to fix the issue while keeping the autocomplete attribute, but if the issue is Safari caching the page, then it would be much better to simply remove the autocomplete attribute to stop the violation reports.

@GrahamCampbell GrahamCampbell changed the title Turn off autocomplete for csrf_field [10.x] Turn off autocomplete for csrf_field Nov 26, 2023
@crochefort
Copy link

@valorin are you referring to this comment from @maxheckel #48371 (comment)

The reason why it's failing the validator is this one here : https://www.w3.org/WAI/standards-guidelines/act/rules/73f2c2/

Applicability
This rule applies to any HTML input, select and textarea element with an autocomplete attribute value that is neither empty ("") nor only ASCII whitespace, except if one or more of the following is true:

So here, the autocomplete give the validation error because our CSRF is hidden.

But from what I have read this morning, Firefox is also doing that, more information here on MDN https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete#sect58 So both browser are not respecting WAI rules as per this mention here : https://www.w3.org/WAI/standards-guidelines/act/rules/

Rules for WCAG 2
The ACT Rules in this section directly relate to WCAG 2 success criteria. These rules have been approved by the Accessibility Guidelines Working Group. They are fully implemented in at least one test tool or methodology.

Autocomplete attribute has valid value

But the end point here is by adding the autocomplete off, you are adding validation error on multiple website around the world while the issue is on the browser side that is not respecting the rules. I would undo this and open bug issue on the browser side.

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

8 participants