Skip to content

Conversation

@TylerHillery
Copy link
Contributor

@TylerHillery TylerHillery commented Jan 23, 2026

What kind of change does this PR introduce?

resolves #757 by adding support for referencing user_metadata field in RLS policies for inserts

What is the current behavior?

Currently user_metadata is not passed into canUpload checks, so RLS policies can't reference it during file uploads.

What is the new behavior?

Now user_metadata is passed through all upload paths (standard HTTP, S3, TUS, multipart) so RLS policies can use it.

Additional context

  • The main change I made was to canUpload to accept userMetatdata. I updated all callers to pass in userMetadata. Because upload gets called in prepareUpload which gets called in upload I also updated all upload calls to ensure userMetadata is passed through. I found it difficult to ensure I caught every cause though, e.g. it was very easy to miss uploadNewObject needed to be updated in the uploadFromRequest. I'm open to feedback to see if there is a more robust solution we could implement that could be caught with typescript types.
  • The part I found most confusing to update was the canUpload checks for multipart uploads. It required changing the order of certain operations around so that user_metadata was available. Wasn't entirely sure if these changes were okay.
  • Added some context as to why I moved userMetadata from the FileUpload interface to the UploadRequest but wasn't sure if this was the right approach so would appreciate some feedback

@coveralls
Copy link

coveralls commented Jan 23, 2026

Pull Request Test Coverage Report for Build 21370655160

Details

  • 58 of 62 (93.55%) changed or added relevant lines in 5 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.003%) to 76.061%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/http/routes/object/getSignedUploadURL.ts 8 10 80.0%
src/http/routes/tus/lifecycle.ts 14 16 87.5%
Files with Coverage Reduction New Missed Lines %
src/storage/protocols/s3/s3-handler.ts 1 84.01%
src/internal/database/tenant.ts 2 83.65%
Totals Coverage Status
Change from base Build 20830462876: 0.003%
Covered Lines: 25517
Relevant Lines: 33265

💛 - Coveralls

@TylerHillery TylerHillery marked this pull request as ready for review January 23, 2026 23:27
@TylerHillery TylerHillery requested a review from a team January 23, 2026 23:27
@fenos
Copy link
Contributor

fenos commented Jan 26, 2026

@TylerHillery awesome! Can we also pass in the metadata into the canUpload?
User metadata is metadata passed by the user where metadata is the file metadata such as size, content-type etc...

@TylerHillery
Copy link
Contributor Author

@TylerHillery awesome! Can we also pass in the metadata into the canUpload? User metadata is metadata passed by the user where metadata is the file metadata such as size, content-type etc...

I thought we couldn't do this because we don't get the objectMetadata until the object has been uploaded. We check canUpload before uploading to object storage to test RLS. Can you confirm if this is the case?

We could switch the order but then we have to ensure to delete the object if user can't upload and we would be do the "expensive" part uploading when we didn't have to. Much quicker to test RLS first and skip uploading if we don't have to.

Here is the upload method on the Uploader class with notes:

  /**
   * Extracts file information from the request and upload the buffer
   * to the remote storage
   * @param request
   * @param options
   */
  async upload(request: UploadRequest) {
   // NOTE: canUpload() gets called inside prepare upload
    const version = await this.prepareUpload(request)

    try {
      const file = request.file

      const s3Key = this.location.getKeyLocation({
        tenantId: this.db.tenantId,
        bucketId: request.bucketId,
        objectName: request.objectName,
      })
        // NOTE: we don't get object metadata until here
      const objectMetadata = await this.backend.uploadObject(
        storageS3Bucket,
        s3Key,
        version,
        file.body,
        file.mimeType,
        file.cacheControl,
        request.signal
      )

      if (request.file.xRobotsTag) {
        objectMetadata.xRobotsTag = request.file.xRobotsTag
      }

      if (file.isTruncated()) {
        throw ERRORS.EntityTooLarge()
      }

      return this.completeUpload({
        ...request,
        version,
        objectMetadata: objectMetadata,
        userMetadata: { ...request.userMetadata },
      })
    } catch (e) {
      await ObjectAdminDelete.send({
        name: request.objectName,
        bucketId: request.bucketId,
        tenant: this.db.tenant(),
        version: version,
        reqId: this.db.reqId,
      })
      throw e
    }
  }

@fenos
Copy link
Contributor

fenos commented Jan 27, 2026

I thought we couldn't do this because we don't get the objectMetadata until the object has been uploaded.
You are correct on this, but potentially we could rely on the:

  • content-length of the request for the file size
  • content-type for the mimeType

Other fields like etag, lastModified, it's fine if they are empty on the first RLS check

.from(bucketName)
.signUploadObjectUrl(objectName, urlPath as string, uploadSignedUrlExpirationTime, owner, {
upsert: request.headers['x-upsert'] === 'true',
userMetadata: userMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't actually support userMetadata on upload signed urls (which we should) so I'm glad you found this.

I believe we need to store the userMetadata in the returned JWT payload; otherwise, this information will be lost during the signedUpload request. We need to be careful, though, if we decide to store the metadata in the JWT, because it can be abused with large payloads. We can for now restrict the amount of data that we can store in the JWT to something like 1MB i think we already have a constant MAX_CUSTOM_METADATA_SIZE, which can be reused (re-refactored as a validation function, potentially in limits.ts)

But we also need to add the second part (on the upload route) to store the userMetadata received from the JWT. This way, the metadata can't be tempered.

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.

Can't use user_metadata in RLS policies when uploading a file

4 participants