Skip to content

fix: add check for shared secret#108

Merged
tripodsan merged 11 commits intomainfrom
shared-secret
Dec 2, 2025
Merged

fix: add check for shared secret#108
tripodsan merged 11 commits intomainfrom
shared-secret

Conversation

@tripodsan
Copy link
Contributor

@tripodsan tripodsan commented Nov 27, 2025

see https://jira.corp.adobe.com/browse/SITES-33249

  • Added check for shared secret for admin api calls (syncadmin, deleteadmin).
  • invoke DocRoom.handleApiCall() directly from edge functions in order to avoid extra routing via url (this doesn't work w/o RPC...)
  • ensured that SharedDoc is destroyed when failed to initialize, in order to avoid hanging timers of the awareness handler

note: when running in the IDE, I still have the tests hanging... I found some of the leaks, but I assume there are still more. I assume those might cause unhandled promise rejections down the road or prevent the workers from being discarded? maybe?

@tripodsan tripodsan requested review from auniverseaway, bosschaert, karlpauls and kptdobe and removed request for karlpauls and kptdobe November 27, 2025 16:30
@github-actions
Copy link

github-actions bot commented Nov 27, 2025

LCOV of commit d03088c during Install, lint, and test #282

Summary coverage rate:
  lines......: 99.2% (2175 of 2193 lines)
  functions..: 96.1% (73 of 76 functions)
  branches...: no data found

Files changed coverage rate:
                  |Lines       |Functions  |Branches    
  Filename        |Rate     Num|Rate    Num|Rate     Num
  ======================================================
  src/edge.js     | 2.3%    398| 0.0%     9|    -      0
  src/shareddoc.js| 3.2%    664| 0.0%    21|    -      0

Comment on lines +65 to +71
// check if shared token is configured and validate
if (env.COLLAB_SHARED_SECRET) {
if (request.headers.get('authorization') !== `token ${env.COLLAB_SHARED_SECRET}`) {
return new Response('Unauthorized', { status: 401 });
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// same signature as the global `fetch()` function, but the request is always sent to the
// object, regardless of the hostname in the request's URL.
return roomObject.fetch(req);
return await roomObject.fetch(req);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this, the catch below is never cought if the error is thrown async.

Comment on lines +474 to +477
destroy() {
super.destroy();
this.awareness.destroy();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure that the awareness is destroyed, too. this stops the internal timer and allows the process to stop

@tripodsan tripodsan marked this pull request as draft November 27, 2025 17:50
Comment on lines +81 to +83
const apiUrl = new URL(doc);
apiUrl.searchParams.set('api', api);
return roomObject.fetch(new Request(apiUrl));
Copy link
Contributor Author

@tripodsan tripodsan Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this worked. but the fetch() api expects a Request (tested on stage as well)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this doc https://developers.cloudflare.com/workers/runtime-apis/fetch/ it expects either: URL, string or Request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they messed this up... fetch(url, init) is used for making requests, but the fetch(request, env, ctx) handler has a different signature. they should have named this differently.

// `controller.storage` provides access to our durable storage. It provides a simple KV
// get()/put() interface.
this.storage = controller.storage;
this.storage = controller?.storage;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the controller is optional below, it should be here, too

@tripodsan tripodsan marked this pull request as ready for review November 27, 2025 18:23
Comment on lines +57 to +58
// clear event handlers
doc.destroy();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure that eventhandlers and timers are removed when doc is removed from cache

Copy link
Contributor

@bosschaert bosschaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@karlpauls karlpauls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great - I had noticed the missing await for the error as well (but forgot about it). Nice!

@tripodsan tripodsan merged commit a79731f into main Dec 2, 2025
3 checks passed
@tripodsan tripodsan deleted the shared-secret branch December 2, 2025 13:11
adobe-bot pushed a commit that referenced this pull request Dec 8, 2025
# 1.0.0 (2025-12-08)

### Bug Fixes

* add check for shared secret ([#108](#108)) ([a79731f](a79731f))
* add span to known HTML tags ([#101](#101)) ([fbf016d](fbf016d))
* do not empty document if error ([#90](#90)) ([a406add](a406add))
* encode HTML brackets ([#100](#100)) ([6c5b5fd](6c5b5fd))
* encode HTML brackets ([#74](#74)) ([015163e](015163e))
* keep content if no main found in html ([#85](#85)) ([9fd25ac](9fd25ac))
* null doc crashes aem2doc ([#84](#84)) ([6197114](6197114))
* ommit stacktraces in error map for non-dev ([#110](#110)) ([1f896ac](1f896ac))
* remove html comments ([#68](#68)) ([cf05aae](cf05aae))

### Features

* disable stack traces on production ([#109](#109)) ([79c40bd](79c40bd))
* Prevent implicit document creation via If-Match header ([#105](#105)) ([b33ae65](b33ae65))
adobe-bot pushed a commit that referenced this pull request Dec 8, 2025
# 1.0.0 (2025-12-08)

### Bug Fixes

* add check for shared secret ([#108](#108)) ([a79731f](a79731f))
* add span to known HTML tags ([#101](#101)) ([fbf016d](fbf016d))
* do not empty document if error ([#90](#90)) ([a406add](a406add))
* encode HTML brackets ([#100](#100)) ([6c5b5fd](6c5b5fd))
* encode HTML brackets ([#74](#74)) ([015163e](015163e))
* keep content if no main found in html ([#85](#85)) ([9fd25ac](9fd25ac))
* null doc crashes aem2doc ([#84](#84)) ([6197114](6197114))
* ommit stacktraces in error map for non-dev ([#110](#110)) ([1f896ac](1f896ac))
* remove html comments ([#68](#68)) ([cf05aae](cf05aae))
* trigger release ([ca061d4](ca061d4))

### Features

* disable stack traces on production ([#109](#109)) ([79c40bd](79c40bd))
* Prevent implicit document creation via If-Match header ([#105](#105)) ([b33ae65](b33ae65))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants