Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add file size restriction for legacy API #561

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
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
3 changes: 2 additions & 1 deletion src/Altinn.Broker.API/Controllers/LegacyFileController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ CancellationToken cancellationToken
FileTransferId = fileTransferId,
Token = legacyToken,
UploadStream = Request.Body,
IsLegacy = true
IsLegacy = true,
ContentLength = Request.ContentLength ?? Request.Body.Length
Comment on lines +80 to +81
Copy link

@coderabbitai coderabbitai bot Oct 28, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider potential memory issues with Request.Body.Length.

The fallback to Request.Body.Length when Request.ContentLength is null could cause memory issues with large files, as it requires reading the entire stream into memory.

Consider this alternative implementation:

-            IsLegacy = true,
-            ContentLength = Request.ContentLength ?? Request.Body.Length
+            IsLegacy = true,
+            ContentLength = Request.ContentLength ?? 
+                throw new ValidationException("Content-Length header is required for file uploads")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
IsLegacy = true,
ContentLength = Request.ContentLength ?? Request.Body.Length
IsLegacy = true,
ContentLength = Request.ContentLength ??
throw new ValidationException("Content-Length header is required for file uploads")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider this or some other error handling?
May be unneccessary if we know all filesizes is smaller

Copy link

Choose a reason for hiding this comment

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

@Andreass2, understood. If we can ensure that all file sizes are small and the Content-Length header is always present, then additional error handling may not be necessary.


✏️ Learnings added
Learnt from: Andreass2
PR: Altinn/altinn-broker#561
File: src/Altinn.Broker.API/Controllers/LegacyFileController.cs:80-81
Timestamp: 2024-10-29T10:43:15.889Z
Learning: In the Altinn Broker API, specifically in `src/Altinn.Broker.API/Controllers/LegacyFileController.cs`, when handling file uploads in the `UploadFileStreamed` method, we can assume that all uploaded files are small and that the `Content-Length` header is always provided. Therefore, additional error handling for missing `Content-Length` is not required.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All the other comments were a bit stupid, but this one is very valid. I'm inclined to close this Pull Request as this is the Legacy API calling us (and file size is restricted there). Main purpose of this PR was to have it explicit in our code too, but not really worth it if it causes memory issues itself.

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other side, we should require content-length header to be set for the normal upload operation.

}, cancellationToken);
return commandResult.Match(
fileId => Ok(fileId.ToString()),
Expand Down
19 changes: 15 additions & 4 deletions src/Altinn.Broker.Application/UploadFile/UploadFileHandler.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using Altinn.Broker.Application.Settings;
using Altinn.Broker.Core.Application;
using Altinn.Broker.Core.Domain;
using Altinn.Broker.Core.Domain.Enums;
using Altinn.Broker.Core.Helpers;
using Altinn.Broker.Core.Repositories;
Expand Down Expand Up @@ -50,14 +51,11 @@ public async Task<OneOf<Guid, Error>> Process(UploadFileRequest request, Cancell
{
return Errors.ServiceOwnerNotConfigured;
};
var maxUploadSize = resource?.MaxFileTransferSize ?? _maxFileUploadSize;
if (request.ContentLength > maxUploadSize)
if (request.ContentLength > GetMaxUploadSize(resource, request.IsLegacy))
{
return Errors.FileSizeTooBig;
}

await fileTransferStatusRepository.InsertFileTransferStatus(request.FileTransferId, FileTransferStatus.UploadStarted, cancellationToken: cancellationToken);

try
{
var checksum = await brokerStorageService.UploadFile(serviceOwner, fileTransfer, request.UploadStream, cancellationToken);
Expand Down Expand Up @@ -102,4 +100,17 @@ public async Task<OneOf<Guid, Error>> Process(UploadFileRequest request, Cancell
return fileTransfer.FileTransferId;
}, logger, cancellationToken);
}

private long GetMaxUploadSize(ResourceEntity resource, bool isLegacy)
{
if (isLegacy)
{
return 1024 * 1000 * 1000; // 1 GB
Ceredron marked this conversation as resolved.
Show resolved Hide resolved
}
if (resource.MaxFileTransferSize is not null)
{
return resource.MaxFileTransferSize.Value;
}
return _maxFileUploadSize;
}
Ceredron marked this conversation as resolved.
Show resolved Hide resolved
}
59 changes: 59 additions & 0 deletions tests/Altinn.Broker.Tests/Helpers/FakeFileStream.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
namespace Altinn.Broker.Tests.Helpers;
public class FakeFileStream : Stream
{
private readonly long _length;
private long _position;

public FakeFileStream(long length)
{
_length = length;
_position = 0;
}

public override bool CanRead => true;
public override bool CanSeek => true;
public override bool CanWrite => false;
public override long Length => _length;
public override long Position
{
get => _position;
set => _position = value;
}

public override int Read(byte[] buffer, int offset, int count)
{
var remainingBytes = _length - _position;
if (remainingBytes <= 0) return 0;

var bytesToRead = (int)Math.Min(count, remainingBytes);
// Fill buffer with dummy data
for (int i = offset; i < offset + bytesToRead; i++)
{
buffer[i] = (byte)(i % 256);
}

_position += bytesToRead;
return bytesToRead;
}
Ceredron marked this conversation as resolved.
Show resolved Hide resolved

public override long Seek(long offset, SeekOrigin origin)
{
switch (origin)
{
case SeekOrigin.Begin:
Position = offset;
break;
case SeekOrigin.Current:
Position += offset;
break;
case SeekOrigin.End:
Position = Length + offset;
break;
}
return Position;
}
Ceredron marked this conversation as resolved.
Show resolved Hide resolved

public override void Flush() { }
public override void SetLength(long value) => throw new NotImplementedException();
public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException();
}
23 changes: 23 additions & 0 deletions tests/Altinn.Broker.Tests/LegacyFileControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,29 @@ public async Task Download_ConfirmDownloaded_Success()
Assert.Equal(LegacyRecipientFileStatusExt.DownloadConfirmed, result?.Recipients[0]?.CurrentRecipientFileStatusCode);
Assert.Equal(LegacyFileStatusExt.AllConfirmedDownloaded, result?.FileStatus);
}

[Fact]
public async Task UploadTooBigFile_ToLegacyApi_FailsWithValidationError()
{
// Initialize
var initializeFileResponse = await _legacyClient.PostAsJsonAsync("broker/api/legacy/v1/file", FileTransferInitializeExtTestFactory.BasicFileTransfer());
string onBehalfOfConsumer = FileTransferInitializeExtTestFactory.BasicFileTransfer().Sender;
Assert.True(initializeFileResponse.IsSuccessStatusCode, await initializeFileResponse.Content.ReadAsStringAsync());
var fileId = await initializeFileResponse.Content.ReadAsStringAsync();
var fileAfterInitialize = await _legacyClient.GetFromJsonAsync<LegacyFileOverviewExt>($"broker/api/legacy/v1/file/{fileId}?onBehalfOfConsumer={onBehalfOfConsumer}", _responseSerializerOptions);
Assert.NotNull(fileAfterInitialize);
Assert.Equal(LegacyFileStatusExt.Initialized, fileAfterInitialize.FileStatus);

// Upload
var fileSize = 1024 * 1000 * 1000 + 1;
var content = new FakeFileStream(fileSize);
var streamContent = new StreamContent(content);
streamContent.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream");
streamContent.Headers.ContentLength = fileSize;
var uploadResponse = await _legacyClient.PostAsync($"broker/api/legacy/v1/file/{fileId}/upload?onBehalfOfConsumer={onBehalfOfConsumer}", streamContent);
Assert.Equal(HttpStatusCode.BadRequest, uploadResponse.StatusCode);
}

private string GetMalwareScanResultJson(string filePath, string fileId)
{
string jsonBody = File.ReadAllText(filePath);
Expand Down
Loading