Preserve Cloudfare HTTP cookies in codex#17783
Conversation
|
|
||
| /// Adds the process-local ChatGPT cookie jar used by Codex HTTP clients. | ||
| /// | ||
| /// The jar is intentionally not persisted to disk. It only preserves cookies for ChatGPT backend |
There was a problem hiding this comment.
This is the key - we do it in memory and no disk persistence so that new CLI/app-server instance get the latest cookies from initial request.
There was a problem hiding this comment.
qq what's the story between app-server requests and the client layer requests today? Would client layer have a different set of cookies and if they did would it pass down to app-server?
There was a problem hiding this comment.
Nope they would use the same one cause most if not all request goes through app server. There are definitely individual requests in the app (i.e. codex web tasks etc) but those are not concerning for now.
There was a problem hiding this comment.
sg. just wasn't sure if that story behind the vs-code requests and app-server requests had evolved
| pub fn is_allowed_chatgpt_host(host: &str) -> bool { | ||
| host == "chatgpt.com" | ||
| || host.ends_with(".chatgpt.com") | ||
| || host == "chat.openai.com" |
There was a problem hiding this comment.
I am not 100% sure if this is needed? Can we confirm?
| host == "chatgpt.com" | ||
| || host.ends_with(".chatgpt.com") |
There was a problem hiding this comment.
these two overlap?
and what about chatgpt-preview-*?
also rust noob question: does rust have a set presence operator?
There was a problem hiding this comment.
these two overlap?
one is strict check and one is looking at the ending so staging.chatgpt.com like url would qualify too.
chatgpt-preview-*
I am not familiar with and I am not sure if we can even point CLI traffic that way?
set presence operator
It most and we can convert to use that.
There was a problem hiding this comment.
one is strict check and one is looking at the ending so staging.chatgpt.com like url would qualify too.
1st comment more around if strict || loose can we omit the strict
preview-*
that's the personal sa-server staging deployment
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
515e4a2 to
8def983
Compare
|
@codex review |
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
8def983 to
40de915
Compare
| normalize_remote_control_url("https://chat.openai.com/backend-api") | ||
| .expect("chat.openai.com URL should normalize"), | ||
| RemoteControlTarget { | ||
| websocket_url: "wss://chat.openai.com/backend-api/wham/remote/control/server" |
There was a problem hiding this comment.
This is for remote control only and I think I will the chagpt_host check consolidation here.
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
40de915 to
ebf719e
Compare
shijie-oai
changed the title
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
We need to make sure everyone uses the codex-client. After quickly checking, I could find cloud-tasks/src/env_detect.rs for example that does not rely on it (which is not good though)
Ok after processing my other comments
|
|
||
| fn is_chatgpt_cookie_url(url: &reqwest::Url) -> bool { | ||
| match url.scheme() { | ||
| "http" | "https" => {} |
There was a problem hiding this comment.
I think this should reject http for non-local ChatGPT hosts for multiple security reasons
| @@ -1,5 +1,6 @@ | |||
| use crate::outgoing_message::OutgoingMessage; | |||
There was a problem hiding this comment.
This couples remote-control endpoint validation to the cookie/client host allowlist. Those are different security boundaries: a host that is safe for Cloudflare cookie replay is not automatically a valid remote-control websocket/enrollment endpoint. Can we keep a remote-control-specific predicate here, or move the shared data into a small policy module that forces each caller to opt into the right use case?
There was a problem hiding this comment.
Sorry did not get to reply earlier - agree with the assessment and I will break it up again.
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
ebf719e to
54e4816
Compare
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
04241a8 to
6f4f15b
Compare
shijie-oai
force-pushed
the
shijie/cf-cookie-preservation-http
branch
from
6f4f15b to
6189c17
Compare
5 participants
Summary