AdguardTeam / CoreLibs

Core Adguard libraries
https://adguard.com/
Apache License 2.0
40 stars 7 forks source link

`maxAge` and `sameSite` doesn't work in `$cookie` rules #1885

Closed AdamWr closed 4 months ago

AdamWr commented 5 months ago

Please answer the following questions for yourself before submitting an issue

AdGuard version

7.18.0 nightly 1 (4744) (CL 1.14.60, DL 2.5.25 )

Browser version

Chrome 125

OS version

Windows 11

Issue Details

Steps to reproduce:

  1. Add this rule:
    $cookie=test;maxAge=30;sameSite=strict,domain=example.org
  2. Navigate to - https://example.org/
  3. Open browser console and run:
    document.cookie = 'test=1'
  4. Refresh website

Expected Behavior

Values for Expires / Max-Age and SameSite should be modified.

Screenshot ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/a70085fa-7357-4681-81bc-19dfd3911342)

Actual Behavior

Values for Expires / Max-Age and SameSite are not modified.

Screenshots

No response

Additional Information

It works fine in with AdGuard extension, so I guess it should also works with apps.

ngorskikh commented 5 months ago

@AdamWr Hello! CoreLibs can only patch Set-Cookie response headers and Cookie request headers, so this is by design.

AdamWr commented 5 months ago

Hello :)

Maybe I don't understand something or understand something incorrectly, but shouldn't it add a Set-Cookie to the Response Headers with value sameSite=strict and appropriate Expires / Max-Age value? The same like it does when just $cookie=test is used?

Screenshot ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/5efb7142-939e-4dc6-9bb0-b73336883e02)

If I add Set-Cookie with test=;sameSite=strict to Response Headers in devtools, then it's set correctly and it's visible in Application tab.

Screenshots ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/43923258-ea3c-40e3-9cb1-26acbd661ed0) ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/223347d9-b55c-4566-b2f8-a5418dc29665)
ngorskikh commented 5 months ago

@AdamWr Sorry, I forgot about this issue :)

Your original test involves setting a cookie through a JS API, hence it doesn't work: there are no headers to modify.

The $cookie modifier must never add cookies -- only modify those that are already present in either the request's Cookie header or the response's Set-Cookie header and whose names match the string or the regex in the modifier's value.

It is weird that you're seemingly able to add Set-Cookie headers to a response with $cookie rules. I can't replicate that, but if it is indeed true, then maybe that is what needs to be fixed :)

AdamWr commented 5 months ago

Your original test involves setting a cookie through a JS API, hence it doesn't work: there are no headers to modify.

That's true, but the last step is to refresh website, and when I refresh website then I see Cookie: test=1 in request headers.

Screenshot ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/7c5a762e-d0f5-4563-8f74-6722b3edef41)

So if I understand correctly, it should be modified by the rule but it isn't.

ngorskikh commented 5 months ago

@AdamWr Ah, I see. The dev tools show the request as it is being sent out by Chrome. To view the request as it arrives at the destination after being modified by AdGuard you'll have to use a MITM proxy or your own HTTP server and make sure that the traffic between it and the browser is filtered by AdGuard.

AdamWr commented 5 months ago

I didn't test it yet as you suggested, but if a cookie is modified by AdGuard then these changes shouldn't be visible in Application tab in devtools (after refreshing website)? The same as it is when I use this rule ($cookie=test;maxAge=30;sameSite=strict,domain=example.org) in AdGuard extension?

Screenshot ![image](https://github.com/AdguardTeam/CoreLibs/assets/29142494/350bf090-310c-4f10-9654-8a768ed19949)
ngorskikh commented 5 months ago

@AdamWr Yes, if all AdGuard could modify was the request, Chrome won't know about the modifications. Furthermore, since the request only carries the cookies' names and values, you actually can't change the cookie's lifetime or any other of its attributes by modifying the request -- only remove the cookie from the request completely.

To sum up, with a CoreLibs-based AdGuard product, the only thing you can do with cookies which are already stored by the browser (either set by JavaScript, or set by a Set-Cookie response header while AdGuard wasn't enabled, or before a rule was added) is remove them from the request's Cookie header.

AdamWr commented 5 months ago

To be honest, I don't understand why it cannot be done. If you can remove cookie by adding Set-Cookie: test=; expires=Thu, 01 Jan 1970 00:00:00 GMT to Response Headers, then why it cannot be modified the same way by adding for example Set-Cookie: test=; Max-Age=5; sameSite=strict, what's the difference?

sfionov commented 4 months ago

@AdamWr We can not add Set-Cookie for corresponding Cookie header for the following two reasons:

  1. This cookie may belong to the upper domain, not the current domain
  2. In some cases this will prolong Cookie lifetime, not reduce it
AdamWr commented 4 months ago

Okay, so I guess that it can be closed.