-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: treat CookieJar write methods as potentially throwing
#3426
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
Changes from all commits
370cb02
544376c
d4cf37a
6b9e246
098b7e4
36d82f7
fd55f27
b81fca2
636757c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { EVENT_SESSION_RETIRED, Session, SessionPool } from '@crawlee/core'; | ||
| import { EVENT_SESSION_RETIRED, log, Session, SessionPool } from '@crawlee/core'; | ||
| import { ResponseWithUrl } from '@crawlee/http-client'; | ||
| import { entries, sleep } from '@crawlee/utils'; | ||
| import { CookieJar } from 'tough-cookie'; | ||
|
|
@@ -207,6 +207,26 @@ describe('Session - testing session behaviour ', () => { | |
| expect(session.getCookieString(url)).toBe('cookie2=your-cookie'); | ||
| }); | ||
|
|
||
| test('setCookies will log warning (not throw) on invalid cookies', () => { | ||
| const url = 'https://www.example.com'; | ||
| // domain 'abc.different.domain' does not match the request URL, so tough-cookie rejects it | ||
| const cookies = [{ name: 'cookie1', value: 'my-cookie', domain: 'abc.different.domain' }]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to explain why this cookie is invalid (it's for a different domain than the one that sets it). Also, can we test calling
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot work on this, add a comment and a new test with a malformed cookie string.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 36d82f7. Added a comment explaining that |
||
|
|
||
| const mockedLog = vitest.mockObject(log, { | ||
| spy: true, | ||
| }); | ||
|
|
||
| session = new Session({ sessionPool, log: mockedLog } as any); | ||
| session.setCookies(cookies, url); | ||
| expect(session.getCookieString(url)).toBe(''); | ||
| expect(mockedLog.warning).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| test('setCookie does not throw on malformed raw cookie string', () => { | ||
| session = new Session({ sessionPool }); | ||
| expect(() => session.setCookie('garbled!!!@#$%nonsense', 'https://www.example.com')).not.toThrow(); | ||
| }); | ||
|
|
||
| test('setCookies works with hostOnly cookies', () => { | ||
| const url = 'https://www.example.com'; | ||
| const cookies = [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only part I'm unsure about is adding
@apify/logas a@crawlee/http-clientdependency... but well, we gotta log somehow.I'm open to ideas here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3399 should resolve this problem, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crawlee/http-clientshould have no dependencies on other Crawlee packages (definitely not@crawlee/core), as it's used from@crawlee/utils- importing@crawlee/corewould create a circular dependency. It's a bit of a predicament :/