Conversation
Code + security review (multi-agent), addressedIndependent code review (Sonnet) and security review (Opus). Security review: Ship (no Critical/High)The trust boundary holds: no path injection ( Code review, fixed
Non-blocking follow-ups
|
e3c8251 to
1c7651f
Compare
|
Rebased onto current main ( Since this branch was cut, main touched the same files five times (highlight settings #325, stream security #327, game-server mono repo #343, demo improvements #346, playback perf #349), so this was worth re-checking rather than trusting a textually clean merge:
|
1c7651f to
11ca622
Compare
|
still sure where i sit with this |
The demo-session pod got S3_PUBLIC_ORIGIN=https://DEMOS_DOMAIN, but
S3Service.getPresignedUrl signs against the demos domain only for the
in-cluster store (rustfs or minio). A remote store is signed against
its own endpoint, path or virtual-host style, so on those installs
every outro URL failed render-clip.mjs's allowlist (it accepts the
outro URLs only when their origin equals S3_PUBLIC_ORIGIN) and every
on-demand clip fell back to the stock outro. Batch highlights were not
affected: their pod gets the outro env directly, without the
allowlist.
S3Service.getPresignedUrlOrigin presigns a probe key for the default
bucket the way resolveOutroBranding does and returns the URL's origin,
so the value follows whatever addressing files-sdk picks instead of
re-implementing it. Checked against files-sdk 2.3.0 and the AWS
presigner: signing is local (the presign middleware returns before the
HTTP handler) and the GET and PUT URLs come from the same client, so
one probe covers the cache GET, the cache PUT and the logo GET. On any
error the pod still gets the demos domain, with a warning.
resolveOutroBranding also sent public.color_dark_tactical_amber raw as
CLIP_BRAND_ACCENT, which game-streamer interpolates into CSS in
headless Chromium. outroAccentFromSetting keeps the setting only when
it is an HSL triple as the web saves it ("33 94% 58%" from its color
picker, "224.3 76.3% 48%" from its defaults) and falls back to
DEFAULT_OUTRO_ACCENT otherwise, so the version hash covers the accent
that is really rendered and game-streamer, which now drops the whole
outro env for an invalid accent, never gets one it must reject. The
pattern is the one game-streamer's outro-env.mjs uses.
d58adf7 to
6a88527
Compare
Part 2 of 2:
api(merge AFTER the game-streamer PR). Renderer: thefeature/branded-outroPR in5stackgg/game-streamer. Full design, both PRs, and merge order: 5stackgg/5stack-panel#514.What
The orchestration half of the branded highlight outro: the api decides the outro cache hit/miss per render and passes branding env to the game-streamer render pod. The renderer PR consumes that env.
Changes
src/matches/game-streamer/outro-branding.ts(new): pure helpers.computeOutroVersion=sha1(brandName|accent|logoEtag).slice(0,12),outroCacheKey=branding/outro_<version>_<dims>_<fps>.mp4,buildOutroEnv,outroAccentFromSetting(the setting when it is an HSL triple like33 94% 58%or224.3 76.3% 48%, trimmed; otherwise the default33 94% 58%), andclipOutputFromSpec/sharedClipOutput(the dims/fps a spec renders at, mirroring game-streamer's job-fields, and the output every spec of a batch shares, ornull).outro-branding.spec.tshas 28 unit tests.game-streamer.service.ts: injectsS3Service;resolveOutroBranding(dims, fps):public.logo_url) is set;public.color_dark_tactical_amberwhen it is an HSL triple, otherwise the default33 94% 58%(viaoutroAccentFromSetting, the same pattern the renderer checksCLIP_BRAND_ACCENTagainst), so the version hash covers the accent that is really rendered;{}(which means baked stock outro) when no logo is set or on any error, so it never breaks a render.Wired into the batch-highlights pod env (pod-level) and the
dispatchClipRenderToPodpayload. The batch pod gets one outro env while each job renders at its own spec output, so the outro is keyed on the output all the jobs share (sharedClipOutput); when they differ, no outro env is sent (stock outro) and a warning is logged.The demo-session pod env also gets
S3_PUBLIC_ORIGIN, the originrender-clip.mjsaccepts the outro URLs from.resolveS3PublicOriginreads it off a real presigned URL (S3Service.getPresignedUrlOrigin): the demos domain for the in-cluster store, the store's own host (bucket subdomain included under virtual-host style) for a remote store, which is what gets remote S3 stores the branded outro on the on-demand path. On any error it warns and falls back to the demos domain so the pod still starts.src/s3/s3.service.ts:getPresignedUrlOrigin(bucket = this.bucket)presigns a probe key throughgetPresignedUrl(typeget, nouseLocal, the routing the outro URLs take) and returnsnew URL(url).origin. Signing is local, so the probe key is never requested; GET and PUT URLs come from the same client and share the origin.src/matches/clips/clips.service.ts: passesoutro_envin the on-demand dispatch payload.No DB migration, no
webchange (reuses the existing branding settings).Env contract (shared with the renderer PR)
Same six keys:
CLIP_OUTRO_URL(hit), orCLIP_OUTRO_RENDER=1+CLIP_OUTRO_PUT_URL+CLIP_BRAND_LOGO_URL+CLIP_BRAND_NAME+CLIP_BRAND_ACCENT(miss).CLIP_BRAND_NAMEmay be empty (keeps the stock wordmark);CLIP_BRAND_ACCENTis always an HSL triple. PlusS3_PUBLIC_ORIGINon the demo-session pod: on the on-demand path the renderer drops the whole outro env when a URL is not at that origin or the accent is not an HSL triple, and renders the stock outro.Merge order
Merge AFTER the
game-streamerPR (which must ship:latestfirst). Safe either way thanks to the stock fallback, but the renderer must be updated before the api turns the feature on.Verification
jest80/80 across the five touched spec files: 28 inoutro-branding.spec.ts; the newS3Serviceorigin test (the probe is signed for the default bucket withoutuseLocal); tworesolveS3PublicOrigintests (the presigned origin, and the warning plus demos-domain fallback) and oneresolveOutroBrandingorchestration test (an injection-string accent setting renders and hashes the stock amber) ingame-streamer.service.spec.ts; the other two specs only gained the constructor argument.tsc -p tsconfig.build.json --noEmitclean; the fulltsconfig.jsonrun has 9 pre-existing errors in 7 untouched spec files, none in the changed files.S3Servicesignatures, DI wiring, failure isolation, the exact env-name contract, and dims/fps consistency between the batch and on-demand paths. This round's change was reviewed again with no blocking findings.Non-blocking follow-ups
Promise.all) on the cold path.{}, hit/miss env, stat-throws degrades). The pure helpers and the invalid-accent orchestration case are tested; the rest of the orchestration was verified by review.