Skip to content

Commit 9ef8c00

Browse files
authored
Code-quality review: correctness fixes, ErrRequestOverloaded, removeElementsLocked, file split (#80)
* Address code-quality review findings (code) Behavioral and structural fixes from the core-package quality review; each comes with a test. No public API removals. - handleBroadcast: reject an HTML-id broadcast target containing a tab or newline before it is written verbatim into a wire frame (Jid<0), where it would corrupt framing. A valid HTML id never contains ASCII whitespace, so reject via reportMisuse rather than escape, satisfying wire.WsMsg.Append's documented no-tab/newline precondition. - /jaws/.tail/<key>: bind the one-shot tail drain to the client IP with the same loopback-aware equalIP check the WebSocket claim path uses, so a leaked key cannot drain (and thereby deny) another client's tail. A mismatch is treated as not found. - Add the exported ErrRequestOverloaded sentinel and wrap the broadcast- and event-channel backpressure cancellation causes with it, so callers can match the teardown reason via context.Cause + errors.Is instead of string-matching. - Extract removeElementsLocked, shared by handleRemove and deleteElementLocked, removing a byte-for-byte duplicated tagMap-cleanup loop. - GenerateHeadHTML: deduplicate extra resources against the built-in CSS path as well as the JS path, so re-listing a built-in is a no-op for both. - Add a regression test that errors.Is(err, ErrEventHandlerPanic) stays panic-safe when the handler panicked with a non-comparable value. * Split jaws.go session and broadcast clusters into their own files Pure code-motion, no behavior change: build, go vet, staticcheck, golangci-lint and the full -race / -tags debug -race suites are unchanged. - Move the Jaws-side session helpers (SessionCount, Sessions, getSessionLocked, getCookieSessionsIDs, GetSession, NewSession, newSession, deleteSession) and the session middleware (sessioner, Jaws.Session) into session.go, which already owns the Session methods. - Move the broadcast/dirt/redirect/alert machinery and the SetInner..JsCall convenience wrappers into a new broadcast.go. jaws.go shrinks from 1395 to 936 lines and now holds the instance lifecycle, request pool, Serve loop, client-IP helpers and HTTP routing.
1 parent f6264b5 commit 9ef8c00

8 files changed

Lines changed: 711 additions & 497 deletions

File tree

‎broadcast.go‎

Lines changed: 347 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,347 @@
1+
package jaws
2+
3+
import (
4+
"bytes"
5+
"cmp"
6+
"encoding/json"
7+
"fmt"
8+
"html/template"
9+
"net/url"
10+
"slices"
11+
"strings"
12+
13+
"github.com/linkdata/jaws/lib/key"
14+
"github.com/linkdata/jaws/lib/tag"
15+
"github.com/linkdata/jaws/lib/what"
16+
"github.com/linkdata/jaws/lib/wire"
17+
)
18+
19+
// Broadcast sends a message to all [Request] values.
20+
//
21+
// It must not be called before the JaWS processing loop ([Jaws.Serve] or
22+
// [Jaws.ServeWithTimeout]) is running. Otherwise this call may block once the
23+
// internal broadcast channel fills.
24+
//
25+
// All convenience helpers on [Jaws] that call Broadcast inherit this requirement.
26+
//
27+
// A nil [wire.Message.Dest] targets every Request; a [key.Key] Dest targets the
28+
// single Request with that identity key, and a zero key is dropped; a string Dest
29+
// is an HTML id accepted by all Requests. Any other Dest is expanded into tags.
30+
//
31+
// A [wire.Message.Dest] that cannot be expanded into tags (an illegal tag type)
32+
// is reported through [Jaws.MustLog], which panics when no [Jaws.Logger] is
33+
// set; with a Logger the error is logged and the message is sent to the
34+
// destinations that did expand.
35+
func (jw *Jaws) Broadcast(msg wire.Message) {
36+
switch dest := msg.Dest.(type) {
37+
case nil: // send to all requests
38+
case key.Key: // send to the request with this identity key
39+
if dest == 0 {
40+
// A recycled producer captured a zeroed key; no live request can match
41+
// (keys are always non-zero), so drop it rather than fall through to
42+
// tag expansion.
43+
return
44+
}
45+
case string: // HTML id (accepted by all requests)
46+
default:
47+
expanded, err := tag.TagExpand(nil, msg.Dest)
48+
jw.MustLog(err)
49+
expanded = jw.dropNonComparableTags(expanded)
50+
switch len(expanded) {
51+
case 0:
52+
// no tags, so no requests will match
53+
return
54+
case 1:
55+
msg.Dest = expanded[0]
56+
default:
57+
msg.Dest = expanded
58+
}
59+
}
60+
select {
61+
case <-jw.Done():
62+
case jw.bcastCh <- msg:
63+
}
64+
}
65+
66+
// dropNonComparableTags returns tags unchanged, or nil after reporting misuse via
67+
// [Jaws.reportMisuse], if any tag is not comparable at runtime.
68+
//
69+
// Expanded tags become map keys in the processing loop (wantMessage's lookup in
70+
// Request.tagMap). TagExpand rejects the known runtime-non-comparable
71+
// struct/array case, and this guard remains as a final defense before a bad Dest
72+
// can panic the Serve goroutine and crash the process.
73+
func (jw *Jaws) dropNonComparableTags(tags []any) []any {
74+
for _, tagValue := range tags {
75+
if cmperr := tag.NewErrNotComparable(tagValue); cmperr != nil {
76+
jw.reportMisuse(fmt.Errorf("jaws: Broadcast: %w", cmperr))
77+
return nil
78+
}
79+
}
80+
return tags
81+
}
82+
83+
// setDirty marks all Elements that have one or more of the given tags as dirty.
84+
func (jw *Jaws) setDirty(tags []any) {
85+
jw.mu.Lock()
86+
// Release the lock with defer so it is freed even if a map insert panics: a tag
87+
// that passed the static comparability check in ensureUsableTag can still be
88+
// non-comparable at runtime (a comparable struct holding e.g. a func in an
89+
// interface field) and panic when used as a map key here.
90+
defer jw.mu.Unlock()
91+
for _, tagValue := range tags {
92+
jw.dirtOrder++
93+
jw.dirty[tagValue] = jw.dirtOrder
94+
}
95+
}
96+
97+
// Dirty marks all [Element] values that have one or more of the given tags as dirty.
98+
//
99+
// Note that if any of the tags implement [tag.TagGetter], it will be called
100+
// with a nil [Request]. Prefer using [Request.Dirty] which avoids this.
101+
//
102+
// A tag that somehow passes expansion but is still not hashable panics the
103+
// calling goroutine when hashed as a map key. The panic is contained to the
104+
// caller (the lock is released and the [Jaws.Serve] loop is unaffected); it is
105+
// not logged-and-dropped the way [Jaws.Broadcast] handles such a
106+
// [wire.Message.Dest], because that hashing happens in the Serve goroutine where
107+
// a panic would crash the process. [Request.Dirty] behaves the same as Dirty
108+
// here.
109+
func (jw *Jaws) Dirty(dirtyTags ...any) {
110+
// Use TagExpand+MustLog rather than MustTagExpand: with a nil Context the
111+
// latter panics on an illegal tag even in production, unlike the sibling
112+
// Request.Dirty and Jaws.Broadcast. Log and continue with the partial result.
113+
expanded, err := tag.TagExpand(nil, dirtyTags)
114+
jw.MustLog(err)
115+
jw.setDirty(expanded)
116+
}
117+
118+
// dirtPair pairs a dirty tag with its insertion-order rank, used by sortedDirtTags
119+
// to order tags without re-reading the order from the map on every comparison.
120+
type dirtPair struct {
121+
tag any
122+
ord int
123+
}
124+
125+
// sortedDirtTags returns the keys of dirty ordered by their (insertion-order) int
126+
// value. It materializes {tag,ord} pairs and sorts on the int, rather than sorting
127+
// a []any with a comparator that does two map lookups per comparison: the latter is
128+
// ~2*N*log2(N) hashed any-map lookups, and this runs under the exclusive jw.mu.
129+
func sortedDirtTags(dirty map[any]int) []any {
130+
pairs := make([]dirtPair, 0, len(dirty))
131+
for k, ord := range dirty {
132+
pairs = append(pairs, dirtPair{tag: k, ord: ord})
133+
}
134+
slices.SortFunc(pairs, func(a, b dirtPair) int { return cmp.Compare(a.ord, b.ord) })
135+
dirt := make([]any, len(pairs))
136+
for i := range pairs {
137+
dirt[i] = pairs[i].tag
138+
}
139+
return dirt
140+
}
141+
142+
func (jw *Jaws) distributeDirt() int {
143+
var reqs []*Request
144+
var dirt []any
145+
146+
jw.mu.Lock()
147+
if len(jw.dirty) > 0 {
148+
dirt = sortedDirtTags(jw.dirty)
149+
clear(jw.dirty)
150+
jw.dirtOrder = 0
151+
reqs = make([]*Request, 0, len(jw.requests))
152+
for _, rq := range jw.requests {
153+
reqs = append(reqs, rq)
154+
}
155+
}
156+
jw.mu.Unlock()
157+
158+
for _, rq := range reqs {
159+
rq.appendDirtyTags(dirt)
160+
}
161+
return len(dirt)
162+
}
163+
164+
// Reload requests all [Request] values to reload their current page.
165+
func (jw *Jaws) Reload() {
166+
jw.Broadcast(wire.Message{
167+
What: what.Reload,
168+
})
169+
}
170+
171+
// isSafeRedirect reports whether rawurl is safe to hand to the browser's
172+
// location.assign, and returns the normalized value to actually send.
173+
//
174+
// Leading and trailing ASCII whitespace and control characters are trimmed, as
175+
// browsers strip them before navigating. Only same-document/relative paths and
176+
// the http and https schemes are permitted; this blocks script-bearing schemes
177+
// such as javascript: and data:, backslashes (which browsers treat as '/'), and
178+
// protocol-relative URLs ("//host/path", "/\host") that would navigate to an
179+
// arbitrary external origin.
180+
func isSafeRedirect(rawurl string) (safe string, ok bool) {
181+
safe = strings.TrimFunc(rawurl, func(r rune) bool { return r <= ' ' })
182+
if strings.ContainsRune(safe, '\\') {
183+
return safe, false
184+
}
185+
if u, err := url.Parse(safe); err == nil {
186+
switch strings.ToLower(u.Scheme) {
187+
case "":
188+
if u.Host == "" && !strings.HasPrefix(safe, "//") {
189+
ok = true
190+
}
191+
case "http", "https":
192+
ok = true
193+
}
194+
}
195+
return
196+
}
197+
198+
// redirectMessage validates url for the browser's location.assign and returns
199+
// the wire.Message to broadcast (Data set to the normalized value); the caller
200+
// sets msg.Dest. If url is unsafe it logs the refusal and returns ok=false. It
201+
// is the single point where the redirect policy and rejection message live, so
202+
// Jaws.Redirect and Request.Redirect cannot drift.
203+
func (jw *Jaws) redirectMessage(url string) (msg wire.Message, ok bool) {
204+
var safe string
205+
if safe, ok = isSafeRedirect(url); ok {
206+
msg = wire.Message{What: what.Redirect, Data: safe}
207+
} else {
208+
_ = jw.Log(fmt.Errorf("jaws: refusing unsafe redirect to %q", url))
209+
}
210+
return
211+
}
212+
213+
// Redirect requests all [Request] values to navigate to the given URL.
214+
//
215+
// The URL is validated to be a relative path or an http/https URL; script-bearing
216+
// schemes such as javascript: and protocol-relative ("//host") URLs are refused
217+
// and logged rather than sent to the browser.
218+
func (jw *Jaws) Redirect(url string) {
219+
if msg, ok := jw.redirectMessage(url); ok {
220+
jw.Broadcast(msg)
221+
}
222+
}
223+
224+
// Alert sends an alert to all [Request] values.
225+
//
226+
// The level argument should be one of Bootstrap's alert levels:
227+
// primary, secondary, success, danger, warning, info, light or dark.
228+
//
229+
// The level and msg are HTML-escaped before being sent, so it is safe to pass
230+
// untrusted text; do not pre-escape it.
231+
func (jw *Jaws) Alert(level, msg string) {
232+
jw.Broadcast(wire.Message{
233+
What: what.Alert,
234+
Data: alertData(level, msg),
235+
})
236+
}
237+
238+
// broadcastTo broadcasts a single wire command to all HTML elements matching
239+
// target. It is the shared body of the public broadcast helpers below, which
240+
// differ only in the What command and how they assemble data.
241+
func (jw *Jaws) broadcastTo(target any, w what.What, data string) {
242+
jw.Broadcast(wire.Message{
243+
Dest: target,
244+
What: w,
245+
Data: data,
246+
})
247+
}
248+
249+
// SetInner sends a request to replace the inner HTML of
250+
// all HTML elements matching target.
251+
func (jw *Jaws) SetInner(target any, innerHTML template.HTML) {
252+
jw.broadcastTo(target, what.Inner, string(innerHTML))
253+
}
254+
255+
// SetAttr sends a request to replace the given attribute value in
256+
// all HTML elements matching target.
257+
//
258+
// The value parameter must be the unescaped logical attribute value. It is sent
259+
// to the browser DOM and used as the value argument to setAttribute().
260+
func (jw *Jaws) SetAttr(target any, attr, value string) {
261+
jw.broadcastTo(target, what.SAttr, attr+"\n"+value)
262+
}
263+
264+
// RemoveAttr sends a request to remove the given attribute from
265+
// all HTML elements matching target.
266+
func (jw *Jaws) RemoveAttr(target any, attr string) {
267+
jw.broadcastTo(target, what.RAttr, attr)
268+
}
269+
270+
// SetClass sends a request to set the given class in
271+
// all HTML elements matching target.
272+
func (jw *Jaws) SetClass(target any, cls string) {
273+
jw.broadcastTo(target, what.SClass, cls)
274+
}
275+
276+
// RemoveClass sends a request to remove the given class from
277+
// all HTML elements matching target.
278+
func (jw *Jaws) RemoveClass(target any, cls string) {
279+
jw.broadcastTo(target, what.RClass, cls)
280+
}
281+
282+
// SetValue sends a request to set the current input value (in textual form) of
283+
// all HTML elements matching target. It sets the live DOM value/state, not the
284+
// HTML "value" attribute.
285+
func (jw *Jaws) SetValue(target any, value string) {
286+
jw.broadcastTo(target, what.Value, value)
287+
}
288+
289+
// Insert calls the JavaScript 'insertBefore()' method on
290+
// all HTML elements matching target.
291+
//
292+
// The position parameter 'where' may be either an HTML ID, a child index or the text "null".
293+
// html is trusted HTML, matching [Jaws.SetInner] and [Jaws.Append].
294+
func (jw *Jaws) Insert(target any, where string, html template.HTML) {
295+
jw.broadcastTo(target, what.Insert, where+"\n"+string(html))
296+
}
297+
298+
// Replace replaces HTML on all HTML elements matching target.
299+
//
300+
// html is trusted HTML, matching [Jaws.SetInner] and [Jaws.Append].
301+
func (jw *Jaws) Replace(target any, html template.HTML) {
302+
jw.broadcastTo(target, what.Replace, string(html))
303+
}
304+
305+
// Delete removes the HTML element(s) matching target.
306+
func (jw *Jaws) Delete(target any) {
307+
jw.broadcastTo(target, what.Delete, "")
308+
}
309+
310+
// Append calls the JavaScript appendChild method on all HTML elements matching target.
311+
func (jw *Jaws) Append(target any, html template.HTML) {
312+
jw.broadcastTo(target, what.Append, string(html))
313+
}
314+
315+
// maybeCompactJSON returns in made safe to embed verbatim in a what.Call wire
316+
// frame, which the client splits on '\n' (frames) and '\t' (order fields). For
317+
// valid JSON, json.Compact strips all insignificant whitespace, including any
318+
// framing-significant tabs and newlines between tokens. When in is not valid JSON
319+
// (for example a caller embedded a raw control byte inside a string literal, which
320+
// is illegal unescaped in JSON), json.Compact fails; rather than passing the raw
321+
// bytes through and corrupting the frame, escape the framing-significant control
322+
// bytes so the payload is always frame-safe (and, as a side effect, valid JSON).
323+
func maybeCompactJSON(in string) string {
324+
if strings.ContainsAny(in, "\n\t") {
325+
var b bytes.Buffer
326+
if err := json.Compact(&b, []byte(in)); err == nil {
327+
return b.String()
328+
}
329+
return jsonControlEscaper.Replace(in)
330+
}
331+
return in
332+
}
333+
334+
// jsonControlEscaper turns control bytes into their JSON escape sequences for the
335+
// fallback path. \n (frame terminator) and \t (field separator) are the only
336+
// framing-significant bytes, matching maybeCompactJSON's detection set; \r is
337+
// escaped solely because a bare \r is illegal inside a JSON string literal, so the
338+
// fallback output stays valid JSON.
339+
var jsonControlEscaper = strings.NewReplacer("\t", `\t`, "\n", `\n`, "\r", `\r`)
340+
341+
var whitespaceRemover = strings.NewReplacer(" ", "", "\n", "", "\t", "")
342+
343+
// JsCall calls the JavaScript function jsfunc with the argument jsonstr
344+
// on all HTML elements matching target.
345+
func (jw *Jaws) JsCall(target any, jsfunc, jsonstr string) {
346+
jw.broadcastTo(target, what.Call, whitespaceRemover.Replace(jsfunc)+"="+maybeCompactJSON(jsonstr))
347+
}

‎errrequestoverloaded.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
package jaws
2+
3+
import "errors"
4+
5+
// ErrRequestOverloaded indicates a [Request] was torn down because it could not keep
6+
// up with the messages addressed to it.
7+
//
8+
// A Request is overloaded when its buffered broadcast channel or its internal
9+
// event-call channel fills before it can drain them. Rather than silently dropping
10+
// messages, which could leave the browser and backend in inconsistent and
11+
// nonreproducible states, the Request is cancelled. The cancellation cause reachable
12+
// via [context.Cause] on [Request.Context] wraps this sentinel, so it can be matched
13+
// with [errors.Is]; the wrapped text identifies which channel overflowed.
14+
var ErrRequestOverloaded = errors.New("request overloaded")

‎eventhandler_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,27 @@ func Test_CallEventHandlers_PanicString(t *testing.T) {
603603
}
604604
}
605605

606+
// Test_CallEventHandlers_PanicNonComparable guards that a handler panicking with a
607+
// non-comparable value (here a map) flows through errors.Is against the exported
608+
// sentinel without panicking. ErrEventHandlerPanic carries a nil Value, so comparing
609+
// the wrapped error against it never reaches a map-to-map comparison; this locks in
610+
// that the reachable matching path stays panic-safe.
611+
func Test_CallEventHandlers_PanicNonComparable(t *testing.T) {
612+
rq := newTestRequest(t)
613+
defer rq.Close()
614+
615+
elem := rq.NewElement(testDivWidget{inner: "x"})
616+
err := CallEventHandlers(testPanicInputHandler{panicVal: map[int]int{1: 1}}, elem, what.Input, "")
617+
if !errors.Is(err, ErrEventHandlerPanic) {
618+
t.Errorf("got %v, want ErrEventHandlerPanic", err)
619+
}
620+
// The non-comparable value is not an error, so Unwrap yields nil rather than
621+
// exposing the map.
622+
if errors.Unwrap(err) != nil {
623+
t.Errorf("Unwrap: got %v, want nil", errors.Unwrap(err))
624+
}
625+
}
626+
606627
func Test_CallEventHandlers_ClickOnlyHandlerViaApplyGetter(t *testing.T) {
607628
rq := newTestRequest(t)
608629
defer rq.Close()

0 commit comments

Comments
 (0)