Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apps/api/plane/api/views/asset.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ def post(self, request):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down Expand Up @@ -336,7 +336,7 @@ def post(self, request):
# Get the presigned URL
storage = S3Storage(request=request, is_server=True)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down Expand Up @@ -563,7 +563,7 @@ def post(self, request, slug):

# Get the presigned URL
storage = S3Storage(request=request, is_server=True)
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)

return Response(
{
Expand Down
2 changes: 1 addition & 1 deletion apps/api/plane/api/views/issue.py
Original file line number Diff line number Diff line change
Expand Up @@ -1931,7 +1931,7 @@ def post(self, request, slug, project_id, issue_id):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down
6 changes: 3 additions & 3 deletions apps/api/plane/app/views/asset/v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ def post(self, request):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down Expand Up @@ -367,7 +367,7 @@ def post(self, request, slug):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down Expand Up @@ -570,7 +570,7 @@ def post(self, request, slug, project_id):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)
# Return the presigned URL
return Response(
{
Expand Down
2 changes: 1 addition & 1 deletion apps/api/plane/app/views/issue/attachment.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def post(self, request, slug, project_id, issue_id):
storage = S3Storage(request=request)

# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size_limit)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size_limit)

# Return the presigned URL
return Response(
Expand Down
71 changes: 71 additions & 0 deletions apps/api/plane/settings/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ def __init__(self, request=None):
self.aws_s3_endpoint_url = os.environ.get("AWS_S3_ENDPOINT_URL") or os.environ.get("MINIO_ENDPOINT_URL")
# Use the SIGNED_URL_EXPIRATION environment variable for the expiration time (default: 3600 seconds)
self.signed_url_expiration = int(os.environ.get("SIGNED_URL_EXPIRATION", "3600"))
# Which presigned upload flavour the browser should use: "post" (default) or "put".
#
# Default stays "post" because that is what AWS S3 and MinIO implement and what every
# existing deployment already uses. Set it to "put" for object stores that do NOT
# implement presigned POST — notably Cloudflare R2, which answers a presigned POST with
# `501 NotImplemented: Presigned post requests are not yet implemented`, so browser
# uploads can never land while server-side flows keep working.
self.upload_method = os.environ.get("AWS_S3_UPLOAD_METHOD", "post").strip().lower()
if self.upload_method not in ("post", "put"):
self.upload_method = "post"

if os.environ.get("USE_MINIO") == "1":
# Determine protocol based on environment variable
Expand Down Expand Up @@ -96,8 +106,69 @@ def generate_presigned_post(self, object_name, file_type, file_size, expiration=
print(f"Error generating presigned POST URL: {e}")
return None

response["method"] = "POST"
return response

def generate_presigned_put(self, object_name, file_type, file_size, expiration=None):
"""Generate a presigned PUT URL to upload an S3 object.

For object stores without presigned POST support (Cloudflare R2). Returns the same
envelope as generate_presigned_post so callers and the client stay uniform:
`url` plus an empty `fields`, with the headers the client must send in `headers`.

The constraints the POST policy expressed as `conditions` are preserved by SIGNING them
as headers — `Content-Type` and `Content-Length` land in SignedHeaders, so the store
rejects any mismatch with 403 SignatureDoesNotMatch. This is STRICTER than the POST
policy it replaces: `content-length-range` allowed anything in [1, file_size], whereas a
signed Content-Length pins the size exactly, and the key is part of the signed URL rather
than a forgeable form field.
"""
if expiration is None:
expiration = self.signed_url_expiration

# A presigned PUT addresses one concrete key, so the POST-only `${filename}` template
# cannot be expressed. No caller uses it, but fail loudly rather than silently upload to
# a key with a literal "${filename}" in it.
if object_name.startswith("${filename}"):
raise ValueError("generate_presigned_put requires a concrete object key; '${filename}' is POST-only")

try:
url = self.s3_client.generate_presigned_url(
"put_object",
Params={
"Bucket": self.aws_storage_bucket_name,
"Key": object_name,
"ContentType": file_type,
"ContentLength": file_size,
},
ExpiresIn=expiration,
)
except ClientError as e:
print(f"Error generating presigned PUT URL: {e}")
return None

return {
"method": "PUT",
"url": url,
# Kept (empty) so the response shape is stable for clients that read `fields`.
"fields": {},
"headers": {"Content-Type": file_type, "Content-Length": str(file_size)},
}

def generate_presigned_upload(self, object_name, file_type, file_size, expiration=None):
"""Generate a presigned browser upload, POST or PUT per AWS_S3_UPLOAD_METHOD.

This is what the asset views call. The client dispatches on the returned `method`, so
switching an install between S3/MinIO and R2 needs no client-side change.
"""
if self.upload_method == "put":
return self.generate_presigned_put(
object_name=object_name, file_type=file_type, file_size=file_size, expiration=expiration
)
return self.generate_presigned_post(
object_name=object_name, file_type=file_type, file_size=file_size, expiration=expiration
)

def _get_content_disposition(self, disposition, filename=None):
"""Helper method to generate Content-Disposition header value"""
if filename is None:
Expand Down
2 changes: 1 addition & 1 deletion apps/api/plane/space/views/asset.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def post(self, request, anchor):
# Get the presigned URL
storage = S3Storage(request=request)
# Generate a presigned URL to share an S3 object
presigned_url = storage.generate_presigned_post(object_name=asset_key, file_type=type, file_size=size)
presigned_url = storage.generate_presigned_upload(object_name=asset_key, file_type=type, file_size=size)
# Return the presigned URL
return Response(
{
Expand Down
115 changes: 115 additions & 0 deletions apps/api/plane/tests/unit/settings/test_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,3 +204,118 @@ def test_explicit_expiration_overrides_default(self, mock_boto3):
mock_s3_client.generate_presigned_url.assert_called_once()
call_kwargs = mock_s3_client.generate_presigned_url.call_args[1]
assert call_kwargs["ExpiresIn"] == 120


S3_ENV = {
"AWS_ACCESS_KEY_ID": "test-key",
"AWS_SECRET_ACCESS_KEY": "test-secret",
"AWS_S3_BUCKET_NAME": "test-bucket",
"AWS_REGION": "us-east-1",
}


@pytest.mark.unit
class TestS3StorageUploadMethod:
"""Test AWS_S3_UPLOAD_METHOD dispatch and the presigned PUT flavour.

Presigned PUT exists for object stores that do not implement presigned POST — Cloudflare R2
answers a presigned POST with 501 NotImplemented, so browser uploads can never land there
while server-side flows keep working.
"""

@patch.dict(os.environ, S3_ENV, clear=True)
@patch("plane.settings.storage.boto3")
def test_upload_method_defaults_to_post(self, mock_boto3):
"""Default must stay POST — that is what S3 and MinIO implement"""
mock_boto3.client.return_value = Mock()
assert S3Storage().upload_method == "post"

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "PUT"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_upload_method_is_case_insensitive(self, mock_boto3):
mock_boto3.client.return_value = Mock()
assert S3Storage().upload_method == "put"

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "sftp"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_unknown_upload_method_falls_back_to_post(self, mock_boto3):
"""An unrecognised value must not silently disable uploads"""
mock_boto3.client.return_value = Mock()
assert S3Storage().upload_method == "post"

@patch.dict(os.environ, S3_ENV, clear=True)
@patch("plane.settings.storage.boto3")
def test_generate_presigned_upload_dispatches_to_post_by_default(self, mock_boto3):
mock_s3_client = Mock()
mock_s3_client.generate_presigned_post.return_value = {"url": "https://s3", "fields": {}}
mock_boto3.client.return_value = mock_s3_client

response = S3Storage().generate_presigned_upload("test-object", "image/png", 1024)

mock_s3_client.generate_presigned_post.assert_called_once()
mock_s3_client.generate_presigned_url.assert_not_called()
assert response["method"] == "POST"

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "put"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_generate_presigned_upload_dispatches_to_put(self, mock_boto3):
mock_s3_client = Mock()
mock_s3_client.generate_presigned_url.return_value = "https://r2/test-object?sig"
mock_boto3.client.return_value = mock_s3_client

response = S3Storage().generate_presigned_upload("test-object", "image/png", 1024)

mock_s3_client.generate_presigned_post.assert_not_called()
assert response["method"] == "PUT"
assert response["url"] == "https://r2/test-object?sig"
# `fields` stays present-but-empty so the response shape is stable for clients
assert response["fields"] == {}

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "put"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_presigned_put_signs_content_type_and_length(self, mock_boto3):
"""Content-Type and Content-Length must be SIGNED.

This is what replaces the POST policy's conditions: both land in SignedHeaders, so the
store rejects a mismatch with 403 rather than accepting a differently-sized or
differently-typed object. It is stricter than content-length-range, which permitted
anything in [1, file_size].
"""
mock_s3_client = Mock()
mock_s3_client.generate_presigned_url.return_value = "https://r2/test-object?sig"
mock_boto3.client.return_value = mock_s3_client

response = S3Storage().generate_presigned_put("test-object", "image/png", 1024)

call_kwargs = mock_s3_client.generate_presigned_url.call_args[1]
assert mock_s3_client.generate_presigned_url.call_args[0][0] == "put_object"
assert call_kwargs["Params"]["ContentType"] == "image/png"
assert call_kwargs["Params"]["ContentLength"] == 1024
assert call_kwargs["Params"]["Key"] == "test-object"
assert call_kwargs["Params"]["Bucket"] == "test-bucket"
assert response["headers"]["Content-Type"] == "image/png"
assert response["headers"]["Content-Length"] == "1024"

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "put"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_presigned_put_uses_default_expiration(self, mock_boto3):
mock_s3_client = Mock()
mock_s3_client.generate_presigned_url.return_value = "https://r2/test-object?sig"
mock_boto3.client.return_value = mock_s3_client

S3Storage().generate_presigned_put("test-object", "image/png", 1024)

assert mock_s3_client.generate_presigned_url.call_args[1]["ExpiresIn"] == 3600

@patch.dict(os.environ, {**S3_ENV, "AWS_S3_UPLOAD_METHOD": "put"}, clear=True)
@patch("plane.settings.storage.boto3")
def test_presigned_put_rejects_filename_template(self, mock_boto3):
"""`${filename}` is a POST-policy feature and cannot be expressed as a PUT.

No caller uses it, but failing loudly beats uploading to a key containing the literal
string "${filename}".
"""
mock_boto3.client.return_value = Mock()

with pytest.raises(ValueError):
S3Storage().generate_presigned_put("${filename}", "image/png", 1024)
30 changes: 22 additions & 8 deletions apps/web/core/services/file-upload.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
*/

import type { AxiosRequestConfig } from "axios";
import axios from "axios";
import axios, { isCancel } from "axios";
// plane imports
import type { TFileUploadRequestOptions } from "@plane/types";
// services
import { APIService } from "@/services/api.service";

Expand All @@ -16,23 +18,35 @@ export class FileUploadService extends APIService {
super("");
}

/**
* Uploads a file to the specified signed URL.
*
* POST sends the multipart form built from the policy fields; PUT sends the raw file with the
* headers that were signed. Pass `requestOptions` from `getFileUploadRequestOptions` — for PUT
* the headers are part of the signature, so sending different ones fails with 403.
*/
async uploadFile(
url: string,
data: FormData,
data: FormData | File,
requestOptions?: TFileUploadRequestOptions,
uploadProgressHandler?: AxiosRequestConfig["onUploadProgress"]
): Promise<void> {
// axios v1 exports CancelToken as a TYPE only; the runtime value lives on the default
// export, so this rule's named-import suggestion does not compile (TS2693).
// oxlint-disable-next-line import/no-named-as-default-member
this.cancelSource = axios.CancelToken.source();
return this.post(url, data, {
headers: {
"Content-Type": "multipart/form-data",
},
const { method = "POST", headers = { "Content-Type": "multipart/form-data" } } = requestOptions ?? {};
const config: AxiosRequestConfig = {
headers,
cancelToken: this.cancelSource.token,
withCredentials: false,
onUploadProgress: uploadProgressHandler,
})
};
const request = method === "PUT" ? this.put(url, data, config) : this.post(url, data, config);
return request
.then((response) => response?.data)
.catch((error) => {
if (axios.isCancel(error)) {
if (isCancel(error)) {
console.log(error.message);
} else {
throw error?.response?.data;
Expand Down
10 changes: 8 additions & 2 deletions apps/web/core/services/file.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import type { AxiosRequestConfig } from "axios";
// plane types
import { API_BASE_URL } from "@plane/constants";
import { getFileMetaDataForUpload, generateFileUploadPayload } from "@plane/services";
import { getFileMetaDataForUpload, generateFileUploadPayload, getFileUploadRequestOptions } from "@plane/services";
import type { EFileAssetType, TFileEntityInfo, TFileSignedURLResponse } from "@plane/types";
import { getAssetIdFromUrl } from "@plane/utils";
// helpers
Expand Down Expand Up @@ -86,6 +86,7 @@ export class FileService extends APIService {
await this.fileUploadService.uploadFile(
signedURLResponse.upload_data.url,
fileUploadPayload,
getFileUploadRequestOptions(signedURLResponse),
uploadProgressHandler
);
await this.updateWorkspaceAssetUploadStatus(workspaceSlug.toString(), signedURLResponse.asset_id);
Expand Down Expand Up @@ -163,6 +164,7 @@ export class FileService extends APIService {
await this.fileUploadService.uploadFile(
signedURLResponse.upload_data.url,
fileUploadPayload,
getFileUploadRequestOptions(signedURLResponse),
uploadProgressHandler
);
await this.updateProjectAssetUploadStatus(workspaceSlug, projectId, signedURLResponse.asset_id);
Expand Down Expand Up @@ -190,7 +192,11 @@ export class FileService extends APIService {
.then(async (response) => {
const signedURLResponse: TFileSignedURLResponse = response?.data;
const fileUploadPayload = generateFileUploadPayload(signedURLResponse, file);
await this.fileUploadService.uploadFile(signedURLResponse.upload_data.url, fileUploadPayload);
await this.fileUploadService.uploadFile(
signedURLResponse.upload_data.url,
fileUploadPayload,
getFileUploadRequestOptions(signedURLResponse)
);
await this.updateUserAssetUploadStatus(signedURLResponse.asset_id);
return signedURLResponse;
})
Expand Down
3 changes: 2 additions & 1 deletion apps/web/core/services/issue/issue_attachment.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import type { AxiosRequestConfig } from "axios";
import { API_BASE_URL } from "@plane/constants";
// plane types
import { getFileMetaDataForUpload, generateFileUploadPayload } from "@plane/services";
import { getFileMetaDataForUpload, generateFileUploadPayload, getFileUploadRequestOptions } from "@plane/services";
import type { TIssueAttachment, TIssueAttachmentUploadResponse, TIssueServiceType } from "@plane/types";
import { EIssueServiceType } from "@plane/types";
// services
Expand Down Expand Up @@ -58,6 +58,7 @@ export class IssueAttachmentService extends APIService {
await this.fileUploadService.uploadFile(
signedURLResponse.upload_data.url,
fileUploadPayload,
getFileUploadRequestOptions(signedURLResponse),
uploadProgressHandler
);
await this.updateIssueAttachmentUploadStatus(workspaceSlug, projectId, issueId, signedURLResponse.asset_id);
Expand Down
Loading
Loading