Conversation
|
Not ready yet, just want CI to start running on this branch. |
karlseguin
left a comment
karlseguin
left a comment
There was a problem hiding this comment.
- I ran the demo tests, and there wasn't 1 OPTIONS request.
- The singleflight is nice, but it might have been a mistake to land it with this PR. It introduces bugs not related to CORS. I'd consider landing Singlelight first.
- Without caching CORS responses, this seems like it could hurt performance more than initially anticipated.
| return raw[0..authority_end]; | ||
| } | ||
|
|
||
| pub fn isSameOrigin(url: [:0]const u8, origin: [:0]const u8) bool { |
There was a problem hiding this comment.
Frame.isSameOrigin should use this (there's a comment there that assumes Protocols are equal, but I'm not sure why that's true).
| owned.credentials = try arena.dupeZ(u8, c); | ||
| } | ||
|
|
||
| if (req.authored_headers.len > 0) { |
There was a problem hiding this comment.
The callsite dupes these in call_arena, and then we dupe it again in transfer.arena. We should rework headers before landing this.Request.headers is a micro-optimization which put the headers in a curl-friendly list from the get go. It was always questionable, and if we're going to iterate + dupe + iterate + dupe, I think it would both be cleaner and more efficient to:
1 - A single header list
2 - The header has a authored: bool flag
3 - The caller sets the headers after newRequest is called so that transfer.arena is available
There was a problem hiding this comment.
Done in #3118
I want to work on referrer policy, and I'll have the same issue.
| .GET, .HEAD => false, | ||
| else => true, | ||
| }; | ||
| if (!is_cross_origin and !is_unsafe_method) { |
There was a problem hiding this comment.
There's two different types of double negative going on here. They can all be eliminated.
const is_same_origin = URL.isSameOrigin(req.cookie_origin, req.url);
const is_safe_method = switch (req.method) {
.GET, .HEAD => true,
else => false,
};
if (is_same_origin and is_safe_method) {
return;
}
| switch (res) { | ||
| .queued => { | ||
| // joined inflight fetch so release it. | ||
| client.arena_pool.release(arena); |
There was a problem hiding this comment.
I think there's a UAF bug in SingleFlight (1) related to this, BUT, as-is there's no reason to even acquire the arena in this case.
(1) Commented in SingleFlight.enter
| pub const EnterResult = enum { initial, queued }; | ||
|
|
||
| pub fn enter(self: *SingleFlight, key: []const u8, transfer: *Transfer, reason: Transfer.ParkedBy) !EnterResult { | ||
| const gop = try self.pending.getOrPut(self.allocator, key); |
There was a problem hiding this comment.
Pretty sure there's no guarantee that key has the necessary lifetime.
| const lp = @import("lightpanda"); | ||
|
|
||
| const http = @import("http.zig"); | ||
| const Request = @import("../browser/webapi/net/Request.zig"); |
|
|
||
| pub const CorsGate = @This(); | ||
|
|
||
| network: *Network, |
| return false; | ||
| } | ||
|
|
||
| pub fn needsPreflight(transfer: *Transfer) bool { |
There was a problem hiding this comment.
Doesn't need to be public (it isn't the only one)
| allocator: Allocator, | ||
| single_flight: SingleFlight, | ||
|
|
||
| pub const Result = enum { allowed, blocked, pending }; |
There was a problem hiding this comment.
Don't think anything returns/uses .blocked.
| return; | ||
| }, | ||
| .initial => { | ||
| errdefer self.single_flight.abort(robots_url); |
There was a problem hiding this comment.
This parks it, but on failure it never gets unparked.
2 participants
This adds a CORS implementation. Still a WIP.