| 06:35 | <cluelessperson> | I want to help, not sure I have the skills to |
| 07:05 | <annevk> | cluelessperson: welcome, did you look at good first issues on GitHub? |
| 07:37 | <cluelessperson> | annevk: will o |
| 07:37 | <cluelessperson> | do |
| 08:41 | <yoav> | annevk: regarding https://github.com/whatwg/fetch/pull/773#discussion_r209604056 what's the best way to tackle the "code smell" part? should I create a separate definition of "same origin request" and make sure that all places use that instead of defining the algorithm on their own? |
| 08:41 | <yoav> | if so, should that be part of that PR or a separate one? |
| 08:42 | <annevk> | yoav: it depends on what kind of check we really want there |
| 08:42 | <annevk> | yoav: and whether a cross-origin request inbetween "poisons" it somehow |
| 08:42 | <yoav> | I don't *think* it does |
| 08:43 | <annevk> | yoav: that does allow for some attacks |
| 08:43 | <yoav> | we want to control info leakage on requests and make sure that there isn't one unless the site opts-in to one |
| 08:43 | <annevk> | yoav: in any event, since this is processing on the response it'd make more sense for it to rely on the existing response filtering setup |
| 08:46 | <yoav> | what do you mean "processing on the response"? We're talking about https://whatpr.org/fetch/773.html#concept-fetch step 1.8, right? |
| 08:50 | <annevk> | yoav: oh hmm, so another problem with that is that we don't run those steps for redirects |
| 08:51 | <annevk> | yoav: these steps are only run once for the entire fetch operation |
| 08:51 | <yoav> | so redirects won't get CH? That could be a problem... |
| 08:52 | <annevk> | yoav: no, they retain the initial CH, even cross-origin (though depends a bit on safelists and such) |
| 08:52 | <yoav> | hmm |
| 08:53 | <annevk> | And yeah, that'll be a problem for no-cors cross-origin at least, esp with service workers and the new restrictions on Headers |
| 08:53 | <annevk> | yoav: so if there's origin restrictions I guess we need to redo the whole setup of this |
| 08:53 | <yoav> | that *might* be fine from a privacy perspective (maybe a redirect would be considered an opt-in), but need to verify that it is |
| 08:54 | <yoav> | Is there somewhere else where that logic can hang from that can handle redirects? |
| 08:54 | <annevk> | I don't think so, given open redirectors and such |
| 08:54 | <annevk> | yoav: main fetch |
| 08:54 | <yoav> | ok, I'll take a look |
| 08:55 | <annevk> | yoav: though maybe it needs to be even deeper in the stack, near HTTP-network fetch, if it depends on the origin whether or not we'll include the headers |
| 09:02 | <annevk> | I'm surprised by this github/fetch issue about making setting URL's query easier having lots of interaction and then nobody escalating that to whatwg/fetch over the past two years to suggest that maybe something should change... |
| 09:07 | <yoav> | annevk: what do you mean by "depends on the origin"? It depends on the origin's opt-in |
| 09:08 | <yoav> | should the request client-hints set cloning also move somewhere further down? |
| 09:08 | <annevk> | yoav: for the second question, I don't think so, the global won't change |
| 09:09 | <yoav> | ok |
| 09:10 | <annevk> | yoav: hmm, with navigation redirects they might, I suppose; |
| 09:10 | <annevk> | well, no |
| 09:12 | <yoav> | looking at main fetch, would an extra step on step 2 work for this? |
| 09:14 | <annevk> | yoav: yeah, I guess that's where it should go if it needs to affect the request |
| 09:14 | <annevk> | (and each subsequent request in case of redirects) |
| 09:15 | <yoav> | annevk: and regarding the same origin check, should I keep the current language and then clean up to have a single definition somewhere? |
| 09:15 | <yoav> | or clean up before this lands? |
| 09:16 | <annevk> | yoav: so wait, I'm not sure if main fetch step 2 is the place |
| 09:17 | <yoav> | ok |
| 09:17 | <annevk> | yoav: it really depends on the behavior you want in the face of redirects |
| 09:17 | <annevk> | yoav: and you haven't clarified that yet |
| 09:17 | <annevk> | yoav: it sounds like it vary on a per-origin basis what headers end up being included? |
| 09:17 | <annevk> | can vary* |
| 09:17 | <yoav> | yeah, it can |
| 09:18 | <annevk> | okay, so then headers need to be added at the last possible moment |
| 09:18 | <annevk> | because the request, including headers, gets cloned |
| 09:18 | <annevk> | however, that'll also affect what the service worker sees |
| 09:19 | <yoav> | hmm |
| 09:19 | <yoav> | so ideally, I want cross-origin redirects to not get CH headers |
| 09:20 | <yoav> | but I do want SW to see the headers (and potentially change behavior based on them) |
| 09:20 | <annevk> | that combination means you'd have to remove headers, which is something we managed to avoid having to do thus far |
| 09:20 | <annevk> | though we will be adding it for POST -> GET redirects |
| 09:21 | <annevk> | it also makes it hard to distinguish between user-set and UA-set headers, but I think CH already had that problem |
| 09:21 | <annevk> | (I'm not sure the current text accounts for it much) |
| 09:22 | <yoav> | it does not |
| 09:22 | <annevk> | e.g., one problem here is that if you decide to remove CH headers, you'll also remove user-set CH headers |
| 09:22 | <annevk> | users might expect those headers to survive their cross-origin redirect though, assuming appropriate CORS opt ins along the way |
| 09:22 | <annevk> | but this would make that impossible |
| 09:22 | <yoav> | yeah... |
| 09:22 | <annevk> | so you gotta figure that out |
| 09:24 | <yoav> | annevk: is there a subset we can land right now and start a separate discussion for the redirection/SW handling? |
| 09:24 | <yoav> | I prefer not to block the pending PRs on figuring this out |
| 09:25 | <yoav> | and then we can discuss this and its privacy/ergonomics/use-case implications in isolation and not part of a huge PR |
| 09:26 | <annevk> | We could replace all the Client Hints bit with issue markers I suppose? |
| 09:26 | <annevk> | bits* |
| 09:27 | <annevk> | It's pretty important for implementers to know the answers to all these questions, esp for security and such |
| 09:28 | <yoav> | annevk: yeah, I'm not suggesting we'll postpone answering those questions, just that it won't block the pending PRs |
| 09:29 | <yoav> | annevk: the currently specified situation is that CH are sent on cross-origin requests |
| 09:29 | <yoav> | that's also the currently shipped state in Chromium |
| 09:29 | <yoav> | although there's intent to fix it, pending on feature-policy opt in |
| 09:30 | <yoav> | so, we could drop that check from the current PR, and start an issue summarizing the trade-offs we discussed here |
| 09:30 | <annevk> | Yeah, I think if we can't figure out a complete solution it'd be best to take it out for now or add issue markers |
| 09:30 | <yoav> | ok, I'll do that |
| 10:02 | <yoav> | annevk: uploaded a new patch to https://github.com/whatwg/fetch/pull/773. Can you take a look? |
| 10:12 | <annevk> | done |
| 10:26 | <yoav> | annevk: thanks! switched to XXX |
| 10:28 | <yoav> | missed the comment about comment message and CORS safelisted headers |
| 10:28 | <yoav> | annevk: the headers are added to the CORS safelisted list at https://github.com/whatwg/fetch/pull/725 |
| 10:45 | <yoav> | annevk: squashed and added a commit message |
| 12:32 | <yoav> | annevk: Regarding https://github.com/whatwg/fetch/pull/773#discussion_r211569017 what other integration is required beyond what we discussed above? Or should I just link to the same issue? |
| 12:33 | <annevk> | yoav: yeah, I guess the same and maybe the more general one? |
| 12:33 | <annevk> | yoav: just to make it clear client hints is half-finished |
| 12:34 | <annevk> | yoav: or is it client-hints now? |
| 12:34 | <yoav> | half finished in what way? |
| 12:35 | <annevk> | yoav: in that there's a lot of open issues? |
| 12:37 | <yoav> | I can link to https://github.com/whatwg/fetch/issues/726 |
| 12:37 | <yoav> | but believe most of what's covered there should be fixed by this PR and the HTML PR that comes with it |
| 12:39 | <yoav> | other than the feature policy bits |
| 12:40 | <annevk> | And that step 6-7 of fetch is not the place to fix this? |
| 12:40 | <annevk> | I also didn't see the secure context restrictions |
| 12:41 | <annevk> | yoav: basically, what I'm saying is that non-Chromium cannot implement this from looking at Fetch |
| 12:41 | <annevk> | yoav: they'll have to reverse engineer Chromium if they want to ship something today |
| 12:41 | <annevk> | yoav: that much should be clear if you read Fetch |
| 12:41 | <annevk> | Well, what should be clear is that Fetch is incomplete |
| 12:42 | <yoav> | annevk: if you have feedback regarding further things that are missing, I'm happy to take a look |
| 12:46 | <annevk> | yoav: apart from the above, I also worry about non-Google commitments, tests, and impact on no-cors |
| 12:47 | <annevk> | yoav: and landing incomplete PRs with no guarantees on when the full model will be defined also makes me a bit uncomfortable |
| 12:48 | <annevk> | at least in software projects where this is done it's usually the maintainer that ends up taking the hit |
| 12:49 | <TimothyGu> | annevk: Have you seen https://github.com/whatwg/html/issues/3885? |
| 12:49 | <annevk> | TimothyGu: I guess I should read it again |
| 12:50 | <TimothyGu> | Yeah the two options are basically not change the URL, or do what replaceState() does |
| 12:50 | <TimothyGu> | I’m happy to implement the proposed spec in Chrome btw |
| 12:51 | <yoav> | annevk: I think tests should be in reasonable shape, given Tarun's latest work (% the redirection case, where we need to better define the behavior first) |
| 12:52 | <yoav> | what's the impact on no-cors? |
| 12:53 | <annevk> | yoav: I'm not sure, but if an origin opted in and then someone makes a no-cors request to that origin, what would/should happen? |
| 12:53 | <annevk> | Domenic: adding operator overloading to JavaScript to avoid a convenience thingy in fetch() strikes me as overkill, but who knows |
| 12:54 | <annevk> | Domenic: fetch() also already exposes a bunch of convenience features (e.g., types of bodies it accepts) |
| 12:54 | <annevk> | TimothyGu: ah okay, I guess this is good then |
| 12:55 | <annevk> | TimothyGu: updated review comment |
| 12:57 | <TimothyGu> | annevk: thanks! |
| 12:59 | <yoav> | annevk: an origin cannot opt-in to receive the hints when it is a 3rd party. The origin delivering the document can opt-in for future navigation requests |
| 12:59 | <yoav> | than the document can opt-in for its subresources |
| 13:00 | <yoav> | currently it's all of them, but we need to fix it to be for same-origin subresources |
| 13:00 | <yoav> | with cross-origin subresources requiring a specific opt-in for their origin (so Feature Policy) |
| 13:00 | <annevk> | yoav: and secure context? |
| 13:00 | <yoav> | oh, of course |
| 13:01 | <annevk> | yoav: yeah so even with feature policy I wouldn't be comfortable adding these headers to no-cors requests and cors without preflight is also somewhat problematic in hindsight |
| 13:02 | <annevk> | yoav: since it's the attacker that determines the feature policy |
| 13:05 | <yoav> | annevk: Is there a way to make them safelisted if UA added, but not user added? |
| 13:06 | <yoav> | I'd be fine with that, assuming it survives SW fetch events |
| 13:06 | <annevk> | yoav: that'd require new infrastructure, including in implementations |
| 13:06 | <annevk> | yoav: I doubt anyone wants to go for that though, as you'd need a side table for all your header lists, and make sure it stays accurate, etc. |
| 13:08 | <Domenic> | annevk: why would you need operator overloading? Just string conversion |
| 13:09 | <Domenic> | annevk: types of bodies isn't a fundamentally new concept, just convenient typing on an existing concept ("body") |
| 13:12 | <annevk> | Domenic: with string conversion it's rather fragile if the input URL already contained a query |
| 13:13 | <Domenic> | annevk: I doubt that's often what people are dealing with, but if it is, there's URL-based conveniences that would make more sense then, e.g. url.withSearchParams(URLSearchParamsInit) -> URL |
| 14:41 | <annevk> | That ends up being a lot more verbose. And apparently this kind of API is also a common offering among libraries so from that perspective it seems worth elevating |
| 14:41 | <annevk> | (It ends up being verbose if the url variable in your example is a relative URL string.) |
| 16:22 | <nox> | Domenic: Adding new query string parameters is something I did often at my former job. |
| 16:23 | <nox> | E.g. adding pagination parameters to an URL with search filters. |
| 16:27 | <TabAtkins> | Yeah, me too. (Or adding query params to a URL that already has a hash on it.) Relying on string-concat to work here is no good. |
| 16:43 | <annevk> | Perhaps URL.withSearch(url, searchParamsInit) that uses the current global object to find a base URL |
| 16:44 | <annevk> | As Krinkle argued on Twitter that'd allow reuse across APIs rather than have something specific for fetch() |
| 16:45 | <Krinkle> | (hi) |
| 16:45 | <annevk> | It's still hard to hit the sweet spot with that kind of API though as it gets kinda verbose |
| 16:47 | <Krinkle> | I think it'd be fine for the method to return URL, but I guess people would prefer it take string, which means it'd have to be static. That way you'd have fetch(URL.withParams(url, {..})) rather than var x = new URL(url); fetch(x.withParams({..}));. |
| 16:48 | <Krinkle> | we'll want to be careful with regards to whether it means merge or replace for search |
| 16:48 | <TabAtkins> | Yeah, alternate constructor sounds fine to me. |
| 17:07 | <annevk> | Domenic: https://github.com/whatwg/participant-data/pull/7 could use a quick look too |
| 17:08 | <annevk> | Domenic: two takeaways there, perhaps we should validate it's an actual user name and perhaps communicate more clearly what string we want to have? |
| 17:08 | <Domenic> | annevk: yeah, agreed... |
| 17:08 | <annevk> | Domenic: I guess I can file an issue against p.w.o and land this? |
| 17:09 | <annevk> | Domenic: would like to unblock the new contributor |
| 17:09 | <Domenic> | annevk: yeah I'll file the issues, I have some thoughts. Please do land though. |
| 17:09 | <annevk> | ah great, thanks |
| 17:09 | <Domenic> | I guess https://github.com/whatwg/participate.whatwg.org/issues/8 is already open |
| 17:18 | <Domenic> | Sent https://github.com/whatwg/participate.whatwg.org/pull/32 as well |
| 19:43 | <TabAtkins> | annevk: That link-defaults issue we just ran into this morning is fixed now. Your manual fix isn't wrong tho, so no need to remove it. It'll just work properly in the future. |
| 20:20 | <devsnek> | what about throw() |
| 20:21 | <TimothyGu> | devsnek: do you need to implement throw? |
| 20:21 | <devsnek> | no idea |
| 20:21 | <devsnek> | thats why i'm asking lol |
| 20:21 | <TimothyGu> | I guess you should |
| 20:21 | <Domenic> | Hmm |
| 20:22 | <Domenic> | I'm not sure what it gives us |
| 20:22 | <Domenic> | I think if you implement it it will just do the same as if it doensn't exist |
| 20:22 | <Domenic> | return() is worthwhile since it releases the lock |
| 20:22 | <Domenic> | Oh wait |
| 20:22 | <Domenic> | Maybe throw() should too? |
| 20:23 | <TimothyGu> | yeah |
| 20:23 | <Domenic> | Hmm throw() is only used by yield* |
| 20:23 | <Domenic> | I'm unsure when throw() would be called in a streams scenario |
| 20:23 | <Domenic> | Someone should write some code that would trigger it and we could turn that into a test if it turns out to be useful |
| 20:27 | <TimothyGu> | so it seems like yield* in async generator will fallback to return() if throw() doesn't exist |
| 20:29 | <Domenic> | Heh right |
| 20:29 | <Domenic> | > NOTE: If iterator does not have a throw method, this throw is going to terminate the yield* loop. But first we need to give iterator a chance to clean up. |
| 20:29 | <TimothyGu> | throw could be used to revive an async iterator after it has thrown an error |
| 20:30 | <Domenic> | but why |
| 20:30 | <TimothyGu> | but I assume once stream errors, it errors forever? |
| 20:30 | <TimothyGu> | so doesn't seem like `throw()` is necessary after all |
| 20:30 | <TimothyGu> | Domenic: weren't you the champion of the proposal |
| 20:30 | <devsnek> | lol |
| 20:31 | <Domenic> | A lot of the esoteric protocol stuff I just did by analogy |
| 20:33 | <TimothyGu> | I feel like we should have a "language feature equality day" |
| 20:34 | <TimothyGu> | all language features are created equal and deserves equal usage, rights, and protection |
| 20:35 | <Domenic> | Nooooo |
| 20:36 | <devsnek> | lol |
| 20:36 | <TimothyGu> | e.g., just because the mismatched end tag is historically discriminated against doesn't mean it should continue being such – and indeed now the HTML parser treats it as a first-class citizen |
| 20:36 | <Domenic> | with, @@species, stop the madness. |
| 22:59 | <devsnek> | is this validish? https://gc.gy/2597346.png |
| 22:59 | <devsnek> | not quite sure how to make %Promise_reject% link either |
| 23:01 | <Domenic> | devsnek: so you should just do what the rest of the doc does and use higher-level stuff like "reject the promise" or "resolve the promise" |
| 23:02 | <Domenic> | But, note that next() must return a promise for { value, done }, not a { value: promise, done } |
| 23:02 | <devsnek> | actually in an error condition |
| 23:02 | <devsnek> | wouldn't it just be a rejected promise with the error |
| 23:03 | <Domenic> | So it should just be "If _read_ is an abrupt completion, return a promise rejected with _read_.[[Value]]" |
| 23:03 | <devsnek> | ye |
| 23:03 | <Domenic> | yeah exactly |
| 23:03 | <Domenic> | I think we already have a helper for this? |
| 23:03 | <Domenic> | https://streams.spec.whatwg.org/#promise-call |
| 23:03 | <Domenic> | No not quite |
| 23:04 | <devsnek> | "a promise rejected with x" is used all over the place |
| 23:04 | <Domenic> | Yep |
| 23:04 | <devsnek> | ReturnRejectedIfAbrupt would be a nice thing to have |
| 23:04 | <devsnek> | but oh well |
| 23:04 | <Domenic> | Eh, a bit too macro-ey |
| 23:05 | <devsnek> | oh and the object literals thing is okay? |
| 23:05 | <MikeSmith> | TabAtkins: I’m wondering, when you thumbs-up a comment like https://github.com/w3c/svgwg/issues/529#issuecomment-414762241 is it because you haven’t taken time to consider that a comment like that might end up really bumming somebody else out, or is it instead because you just don’t care |
| 23:05 | <Domenic> | Oh, no, it is not. Use https://streams.spec.whatwg.org/#readable-stream-create-read-result |
| 23:06 | <devsnek> | i was afraid of that haha |
| 23:07 | <TabAtkins> | MikeSmith: I've been vocal in the past with my annoyance for putting the CSS validator in the way of publishing, for exactly the reasons Chris cites. |
| 23:09 | <TimothyGu> | devsnek: also nit: put a colon after "If …, then" |
| 23:09 | <devsnek> | i need syntax highlighting for bs |
| 23:09 | <TimothyGu> | :set syntax=html |
| 23:09 | <devsnek> | that won't highlight the algorithms |
| 23:09 | <devsnek> | even if some of it is prose |
| 23:10 | <TimothyGu> | TabAtkins: are there syntax highlighters for bikeshed? |
| 23:11 | <MikeSmith> | TabAtkins: my question wasn’t as much about the substance of Chris’s comment as it was about whether in general you actually take time think about how somebody else might feel when you thumbs-up a comment like that, or make a comment like that yourself |
| 23:11 | <MikeSmith> | because you seem like a better person that somebody who would do that wilfully |
| 23:14 | <TabAtkins> | I can tell you've taken it far more personally than I expected, and I'm very sorry that I've made you feel like this; that absolutely wasn't my intent and I would have done differently if I'd known. Apologies. |
| 23:15 | <TabAtkins> | TimothyGu: In the repo there's a tmLanguage file |
| 23:15 | <TabAtkins> | It highlights a couple useful bits; PRs accepted. |
| 23:15 | <TimothyGu> | ah, no .vim I assume? |
| 23:16 | <TabAtkins> | PRs. Accepted. |
| 23:16 | <TimothyGu> | k |
| 23:25 | <Domenic> | annevk: tomorrow can you take a look at my counter-review of https://github.com/whatwg/html/pull/3677 and let us know which parts you think are important despite my protestations? Also I know there are some things I didn't disagree with that fserb should still fix. Maybe just thumbs up/down would be fine. |
