Fix returning S3 credentials from /api/transcode #1574

leszko · 2023-01-02T10:11:19Z

Redact S3 credentials while calling /api/transcode.

vercel · 2023-01-02T10:11:21Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
livepeer-studio	✅ Ready (Inspect)	Visit Preview	Jan 2, 2023 at 10:13AM (UTC)

codecov · 2023-01-02T10:23:47Z

Codecov Report

Merging #1574 (5d4448c) into master (3171e19) will not change coverage.
The diff coverage is 100.00000%.

@@              Coverage Diff              @@
##              master       #1574   +/-   ##
=============================================
  Coverage   53.70410%   53.70410%           
=============================================
  Files             70          70           
  Lines           4657        4657           
  Branches         884         884           
=============================================
  Hits            2501        2501           
  Misses          1838        1838           
  Partials         318         318

Impacted Files	Coverage Δ
packages/api/src/controllers/task.ts	`25.64103% <ø> (-3.04749%)`	⬇️
packages/api/src/controllers/helpers.ts	`55.55556% <100.00000%> (+1.26985%)`	⬆️
packages/api/src/controllers/transcode.ts	`90.90909% <100.00000%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3171e19...5d4448c. Read the comment docs.

Impacted Files	Coverage Δ
packages/api/src/controllers/task.ts	`25.64103% <ø> (-3.04749%)`	⬇️
packages/api/src/controllers/helpers.ts	`55.55556% <100.00000%> (+1.26985%)`	⬆️
packages/api/src/controllers/transcode.ts	`90.90909% <100.00000%> (ø)`

victorges · 2023-01-03T19:09:59Z

packages/api/src/controllers/transcode.ts

@@ -44,7 +48,7 @@ app.post(
      null,
      req.user.id
    );
-    res.json(task);
+    res.json(taskWithoutCredentials(task));


The pattern that we have established on the API is to just delete the "write-only" fields from responses. This should be done by:

Tagging the field writeOnly in the schema: https://github.com/livepeer/livepeer-com/blob/3171e199ee4887ed5c80943a48162ba2bc70b19b/packages/api/src/schema/schema.yaml#L948

Call db.<table>.cleanWriteOnlyResponse on the object: https://github.com/livepeer/livepeer-com/blob/3171e199ee4887ed5c80943a48162ba2bc70b19b/packages/api/src/controllers/task.ts#L105

Notice that we use the mung middleware above so we don't have to repeat the clean-up logic on every API handler. Feel free to just inline a call to db.tas.cleanWriteOnlyResponse here though.

As this is not a behavioral regression tho, the same helper used before was just moved to being used here as well, I think it's fine as is if you prefer the current way. Would be good to have the consistency across APIs tho.

I think the writeOnly pattern is not applicable here, since we actually do want to return the url field (just without credentials). We could introduce a new label in the schema (like redact or containsPassword), but I believe it's too much overhead right now, so I'm merging it as it is.

Saying that, we can think about some more global redact mechanism for studio. It's actually related to the credentials issue I described here (and would love to get your feedback on it as well): https://www.notion.so/livepeer/S3-Credentials-in-logs-0f038d4ea5a54d24b65634fa6dd10267

I think overall it's not that important from an interface perspective to return the URL field, as the user has just made a request with that as the input, so they have all the data necessary for the URL, and they are not going to use the internal Object Store URL for anything anyway (we even abstracted it on their input). In fact, we do not want users depending on this URLs publicly or we'll lose the ability to iterate on them freely as an internal abstraction.

This is also almost the exact same case as the other places where we do write-only filtering, where it could even be useful to return a part of the data to the user (e.g. return the multistream URLs only removing the secret stream key), but we opt for a simple but consistent approach of removing those fields from the response instead. So a write-only field does seem appropriate to me here as a similar use case to the others we already have. I do agree that a "redact/containsPassword" special element would be overkill though, especially as there's no universal way of determining where the secrets would be in the URLs, so makes sense to keep this ad-hoc implementations instead.

victorges

Seems like a less important filtering since we were only returning the credentials as a response to the same request that created the task with the same credentials (right?).

Either way LGTM.

victorges · 2023-01-03T19:11:50Z

packages/api/src/controllers/transcode.ts

@@ -44,7 +48,7 @@ app.post(
      null,
      req.user.id
    );
-    res.json(task);
+    res.json(taskWithoutCredentials(task));


As this is not a behavioral regression tho, the same helper used before was just moved to being used here as well, I think it's fine as is if you prefer the current way. Would be good to have the consistency across APIs tho.

* Add /api/transcode API (#1548) * Fix returning S3 credentials from /api/transcode (#1574) * api: Upgrade tus-node-server from 0.6.0 to 0.9.0 (#1576) * api: Upgrade tus-node-server from 0.6.0 to 0.9.0 * api: Fix build for new tus lib * chore(release): publish v0.12.0 * graphql to groq (#1573) * feat: switch graphql to groq * chore: * bug: sanity client refactor * bug: client import fix * www: Fix asset details not showing on asset page (#1581) Only missed a details=1 on the requests. * bug: portable text disappered fix (#1589) * Fix all the Next.js warning and errors in the console (#1594) * Fixed the Stripe initialisation * Fixed the payment popup which was throwing an error * Fixed the logo svg width error * Fixed the error with the mobile menu where auth buttons where rendered differently on server and client * Fixed the inclusion of two scripts in the Head that were throwing a warnings * Refactored the Document component to a function component for future work * Fixed the not-unique key in admin table * Fixed the non unique key in admin usage table * Optimised the strip initialisation with useMemo * Enabling SWC and fixing Admin tables (#1595) * Fixed the Stripe initialisation * Fixed the payment popup which was throwing an error * Fixed the logo svg width error * Fixed the error with the mobile menu where auth buttons where rendered differently on server and client * Fixed the inclusion of two scripts in the Head that were throwing a warnings * Refactored the Document component to a function component for future work * Fixed the not-unique key in admin table * Fixed the non unique key in admin usage table * Enabled SWC for the project Fixed the Admin table error * Optimised the strip initialisation with useMemo * Link resolver (#1556) * Test bump * Hotfix: Cleaning up campaign page * feat: custom link resolver * chore: target link * Ran a build, bump * bump: yarn lock * bump: removed token from sanity client Co-authored-by: Jonathan Alford <jono@roboto.studio> * packages/api: Add version information to prometheus metrics (#1597) * Pricing page (#1596) * Test bump * Hotfix: Cleaning up campaign page * Added components * Started to wire components up * feat: image on social proof * Cleaned up 2 sections * Pricing cards nearly there * Cleaned up pricing card grid sizing and close to finish * Fixed spacing * Also cleared all console log errors Co-authored-by: hrithikroboto <hrithik@roboto.studio> Co-authored-by: Rafał Leszko <rafal@livepeer.org> Co-authored-by: Victor Elias <victorgelias@gmail.com> Co-authored-by: hrithikroboto <114240510+hrithikroboto@users.noreply.github.com> Co-authored-by: clacla <claudio@livepeer.org> Co-authored-by: hjpotter92 <hjpotter92@users.noreply.github.com> Co-authored-by: hrithikroboto <hrithik@roboto.studio>

Fix returning S3 credentials from /api/transcode

5d4448c

leszko requested a review from a team as a code owner January 2, 2023 10:11

leszko requested a review from victorges January 2, 2023 10:11

vercel bot deployed to Preview January 2, 2023 10:13 View deployment

victorges reviewed Jan 3, 2023

View reviewed changes

victorges approved these changes Jan 3, 2023

View reviewed changes

leszko merged commit 97c09be into master Jan 4, 2023

leszko deleted the rafal/fix-transcode-api-return-no-credentials branch January 4, 2023 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix returning S3 credentials from /api/transcode #1574

Fix returning S3 credentials from /api/transcode #1574

leszko commented Jan 2, 2023

vercel bot commented Jan 2, 2023 •

edited

codecov bot commented Jan 2, 2023

victorges Jan 3, 2023

victorges Jan 3, 2023

leszko Jan 4, 2023

victorges Jan 4, 2023

victorges left a comment •

edited

victorges Jan 3, 2023

Fix returning S3 credentials from /api/transcode #1574

Fix returning S3 credentials from /api/transcode #1574

Conversation

leszko commented Jan 2, 2023

vercel bot commented Jan 2, 2023 • edited

codecov bot commented Jan 2, 2023

Codecov Report

victorges Jan 3, 2023

Choose a reason for hiding this comment

victorges Jan 3, 2023

Choose a reason for hiding this comment

leszko Jan 4, 2023

Choose a reason for hiding this comment

victorges Jan 4, 2023

Choose a reason for hiding this comment

victorges left a comment • edited

Choose a reason for hiding this comment

victorges Jan 3, 2023

Choose a reason for hiding this comment

vercel bot commented Jan 2, 2023 •

edited

victorges left a comment •

edited