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

WAFastCGIAdaptor doesn’t write cookie attributes ‘Max-Age’ and ‘SameSite’ #1375

Closed
Rinzwind opened this issue Sep 1, 2023 · 2 comments · Fixed by #1378
Closed

Comments

@Rinzwind
Copy link
Member

Rinzwind commented Sep 1, 2023

On WACookie, #writeOn:, through #rfc6265String, writes out ‘Max-Age’ and ‘SameSite’ attributes:

rfc6265String
"Serializes the receiver according to RFC 6265.
See class comment for a link to the spec."
^ String streamContents: [ :stream |
| useQuotes |
"the spec allows us to quote but we don't know how good browser support is"
useQuotes := false.
self writeKeyValueQuoted: useQuotes on: stream.
self writeExpiresOn: stream.
self writeMaxAgeQuoted: useQuotes on: stream.
self writeDomainQuoted: useQuotes on: stream.
self writePathQuoted: useQuotes on: stream.
self writeSecureOn: stream.
self writeHttpOnlyOn: stream.
self writeSameSiteOn: stream ]

But on WAFastCGIAdaptor, #addCookie:toStream: does not write out those attributes:

aWACookie
writeExpiresOn: aStream;
writePathQuoted: false on: aStream;
writeDomainQuoted: false on: aStream;
writeSecureOn: aStream;
writeHttpOnlyOn: aStream.

I’m not sure why it doesn’t just use the WACookie’s #writeOn:.

@jbrichau
Copy link
Member

The change to rfc6265 was never properly implemented in the FastCGI adapter. The current code is there because of #1115

It seems we should indeed simply call writeOn: and remove the old code.

@jbrichau
Copy link
Member

In #1378, I call the rfc6265String because this is also the method being used in the Zinc Adapter.

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 a pull request may close this issue.

2 participants