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(azure): fix cookie format normalization #1753

Merged
merged 6 commits into from
Sep 26, 2023
Merged

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Sep 23, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Cookies set by the users were dropped when the app is deployed to Azure. Reason is that azure expects the cookie to be of the form { name: 'key', value: 'val } but nitro was passing it in the form { key: 'val' }.
This is fixed, along with a few other improvements. (Refs #1452)

While working on this, I found a bug in the cookie parser: unjs/cookie-es#17

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #1753 (e289aba) into main (3f7faa5) will increase coverage by 0.13%.
The diff coverage is 92.72%.

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

@@            Coverage Diff             @@
##             main    #1753      +/-   ##
==========================================
+ Coverage   77.26%   77.39%   +0.13%     
==========================================
  Files          75       76       +1     
  Lines        7912     7976      +64     
  Branches      806      823      +17     
==========================================
+ Hits         6113     6173      +60     
- Misses       1797     1801       +4     
  Partials        2        2              
Files Coverage Ξ”
src/runtime/utils.azure.ts 93.75% <92.72%> (ΓΈ)

tobiasdiez added a commit to JabRef/JabRefOnline that referenced this pull request Sep 24, 2023
- Once redis/ioredis#1822 is fixed, remove
patch to ioredis
- Once Azure/azure-functions-host#162 is
fixed, remove patch to nitro that wraps console log.
- Once nitrojs/nitro#1753 is merged and released,
remove corresponding patch to nitro
- Once lucia-auth/lucia#1153 is fixed, rename
models/fields in prisma
- Once lucia-auth/lucia#1155 is merged and
released, remove custom h3 lucia middleware
- Once lucia-auth/lucia#1074 is fixed, remove
lucia types shims
- Enable CSRF protection in lucia (but then login doesn't work
anymore...)
@pi0 pi0 changed the title fix: improve conversation of azure cookies fix(azure): fix cookie format normalization Sep 26, 2023
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks for this nice PR and also unit tests πŸ’―

I have added few refactors to have basic httponly/secure support, one typo in sameSite case and also more generic code style.

If something was bad, feel free to amend via a new PR.

@pi0 pi0 merged commit aee0204 into nitrojs:main Sep 26, 2023
4 of 5 checks passed
@tobiasdiez tobiasdiez deleted the azure_cookie branch September 27, 2023 02:17
@tobiasdiez
Copy link
Contributor Author

Thanks @pi0, your changes are nice improvements!

Btw, I think it would make sense to put this cookie parsing actually in h3, and use it to internally store the cookies as nice typed objects. What happens right now if you set a cookie is the following: set cookie as object > serialize cookie to string > now here in the azure handler: parse cookie again to get it back as an object. This whole serialization and parsing business can be skipped for azure and a few other frameworks. As a bonus, you get a nicer api in h3 to handle cookies.

@pi0
Copy link
Member

pi0 commented Sep 27, 2023

For now, i think we can also expose it from cookie-es. It was initially for compatibility but seems the API is also not that nice and could be better. PR is welcome πŸ‘πŸΌ

@tobiasdiez
Copy link
Contributor Author

Makes sense. I don't have the time right now, but added it to unjs/h3#509 and unjs/cookie-es#17 so we keep track of it.

@pi0
Copy link
Member

pi0 commented Sep 29, 2023

Perfect! Thanks for creating issues.

@pi0 pi0 mentioned this pull request Oct 5, 2023
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.

2 participants