Add optional mode argument to persist_uploaded_file (#1241)

* Add optional mode argument to persist_uploaded_file

This change adds an optional `mode` argument to the `persist_uploaded_file` function, allowing users to specify the Unix file permissions in octal notation when saving uploaded files.

- Updated `sqlpage.persist_uploaded_file` signature to include `mode`.
- Implemented permission setting logic using `std::os::unix::fs::PermissionsExt` (on Unix platforms).
- Default permission is set to "600" (octal `0o600`).
- Added documentation for the new parameter in `examples/official-site/sqlpage/migrations/39_persist_uploaded_file.sql`, including an explanation of octal notation and a link to Wikipedia.
- Added a unit test `test_set_file_mode` to verify the permission setting logic.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Address PR feedback: Add integrated test for persist_uploaded_file mode

- Removed unit test from `functions.rs` and added an integrated test in `tests/uploads/mod.rs`.
- Created `tests/uploads/persist_with_mode.sql` for the integrated test.
- Refactored `set_file_mode` to use `#[cfg(unix)]` and `#[cfg(not(unix))]` on the entire function.
- Replied to PR comments.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Fix Windows CI: Normalize paths and improve tests

- Normalized `persist_uploaded_file` return path to use forward slashes for URL compatibility.
- Updated `test_persist_uploaded_file_mode` to handle platform-specific path separators.
- Fixed clippy warning `expect_fun_call` in tests.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Address PR feedback: Assert file contents and ignore test uploads

- Added assertion to verify persisted file contents in `test_persist_uploaded_file_mode`.
- Removed accidental test file from git and added `tests_uploads/` to `.gitignore`.
- Replied to PR comments.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Address PR feedback: Revert breaking change to return value

- Reverted normalization of `persist_uploaded_file` return value to avoid a breaking change.
- Reverted corresponding test changes that relied on normalized paths.
- Replied to PR comments.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Address PR feedback: Query results as JSON in integrated test

- Updated `test_persist_uploaded_file_mode` to directy request and verify JSON results.
- Replied to PR comments.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Fix Windows CI: Robust path handling in tests

- Improved integrated test to correctly request JSON results.
- Added platform-specific path normalization when verifying files on disk.
- Ensured `persist_uploaded_file` return value remains OS-specific to avoid breaking changes.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

* Address PR feedback: Delete leftover test file

- Deleted accidental leftover test file `tests_uploads/2026-03-13_15h47m26s_6JaXODDK.txt`.
- Replied to PR comments.

Co-authored-by: lovasoa <552629+lovasoa@users.noreply.github.com>

---------

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This commit is contained in:
Ophir LOJKINE
2026-03-13 19:57:38 +01:00
committed by GitHub
parent 3eb527490e
commit 99ad2685a0
5 changed files with 92 additions and 1 deletions
+1
View File
@@ -11,3 +11,4 @@ xbed.sql
**/sqlpage.bin
node_modules/
sqlpage/sqlpage.db
tests_uploads/
@@ -62,4 +62,14 @@ VALUES (
'Optional. Comma-separated list of allowed file extensions. By default: jpg,jpeg,png,gif,bmp,webp,pdf,txt,doc,docx,xls,xlsx,csv,mp3,mp4,wav,avi,mov.
Changing this may be dangerous ! If you add "sql", "svg" or "html" to the list, an attacker could execute arbitrary SQL queries on your database, or impersonate other users.',
'TEXT'
),
(
'persist_uploaded_file',
4,
'mode',
'Optional. Unix permissions to set on the file, in octal notation. By default, the file will be saved with "600" (read/write for the owner only).
Octal notation works by using three digits from 0 to 7: the first for the owner, the second for the group, and the third for others.
For example, "644" means read/write for the owner, and read-only for others.
[Learn more about numeric notation for file-system permissions on Wikipedia](https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation).',
'TEXT'
);
@@ -42,7 +42,7 @@ super::function_definition_macro::sqlpage_functions! {
link(file: Cow<str>, parameters: Option<Cow<str>>, hash: Option<Cow<str>>);
path((&RequestInfo));
persist_uploaded_file((&RequestInfo), field_name: Cow<str>, folder: Option<Cow<str>>, allowed_extensions: Option<Cow<str>>);
persist_uploaded_file((&RequestInfo), field_name: Cow<str>, folder: Option<Cow<str>>, allowed_extensions: Option<Cow<str>>, mode: Option<Cow<str>>);
protocol((&RequestInfo));
random_string(string_length: SqlPageFunctionParam<usize>);
@@ -420,6 +420,7 @@ async fn persist_uploaded_file<'a>(
field_name: Cow<'a, str>,
folder: Option<Cow<'a, str>>,
allowed_extensions: Option<Cow<'a, str>>,
mode: Option<Cow<'a, str>>,
) -> anyhow::Result<Option<String>> {
let folder = folder.unwrap_or(Cow::Borrowed("uploads"));
let allowed_extensions_str =
@@ -456,6 +457,7 @@ async fn persist_uploaded_file<'a>(
target_path.display()
)
})?;
set_file_mode(&target_path, mode.as_deref()).await?;
// remove the WEB_ROOT prefix from the path, but keep the leading slash
let path = "/".to_string()
+ target_path
@@ -475,6 +477,26 @@ async fn protocol(request: &RequestInfo) -> &str {
&request.protocol
}
#[cfg(unix)]
async fn set_file_mode(path: &std::path::Path, mode: Option<&str>) -> anyhow::Result<()> {
use std::os::unix::fs::PermissionsExt;
let mode = if let Some(mode) = mode {
u32::from_str_radix(mode, 8)
.with_context(|| format!("unable to parse file mode {mode:?} as an octal number"))?
} else {
0o600
};
tokio::fs::set_permissions(path, std::fs::Permissions::from_mode(mode))
.await
.with_context(|| format!("unable to set permissions on {}", path.display()))?;
Ok(())
}
#[cfg(not(unix))]
async fn set_file_mode(_path: &std::path::Path, _mode: Option<&str>) -> anyhow::Result<()> {
Ok(())
}
/// Returns a random string of the specified length.
pub(crate) async fn random_string(len: usize) -> anyhow::Result<String> {
// OsRng can block on Linux, so we run this on a blocking thread.
+56
View File
@@ -28,6 +28,62 @@ async fn test_file_upload(target: &str) -> actix_web::Result<()> {
Ok(())
}
#[actix_web::test]
async fn test_persist_uploaded_file_mode() -> actix_web::Result<()> {
let app_data = crate::common::make_app_data().await;
let req = actix_web::test::TestRequest::get()
.uri("/tests/uploads/persist_with_mode.sql?mode=644")
.app_data(app_data.clone())
.app_data(sqlpage::webserver::http::payload_config(&app_data))
.app_data(sqlpage::webserver::http::form_config(&app_data))
.insert_header((actix_web::http::header::ACCEPT, "application/json"))
.insert_header(("content-type", "multipart/form-data; boundary=1234567890"))
.set_payload(
"--1234567890\r\n\
Content-Disposition: form-data; name=\"my_file\"; filename=\"test.txt\"\r\n\
Content-Type: text/plain\r\n\
\r\n\
Hello\r\n\
--1234567890--\r\n",
)
.to_srv_request();
let resp = main_handler(req).await?;
assert_eq!(resp.status(), StatusCode::OK);
let body_json: serde_json::Value = test::read_body_json(resp).await;
let persisted_path = body_json[0]["contents"]
.as_str()
.expect("Path not found in JSON response");
// The path is relative to web root, we need to find it on disk.
// In tests, web root is the repo root.
// We normalize the path separators to be platform-specific for the file system check.
let normalized_path = persisted_path
.replace('\\', "/")
.trim_start_matches('/')
.replace('/', std::path::MAIN_SEPARATOR_STR);
let file_path = std::path::Path::new(&normalized_path);
assert!(
file_path.exists(),
"Persisted file {} does not exist. Persisted path: {}, JSON: {}",
file_path.display(),
persisted_path,
body_json
);
let contents = std::fs::read_to_string(file_path)?;
assert_eq!(contents, "Hello");
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let metadata = std::fs::metadata(file_path)?;
assert_eq!(metadata.permissions().mode() & 0o777, 0o644);
}
std::fs::remove_file(file_path)?;
Ok(())
}
#[actix_web::test]
async fn test_file_upload_direct() -> actix_web::Result<()> {
test_file_upload("/tests/uploads/upload_file_test.sql").await
+2
View File
@@ -0,0 +1,2 @@
set contents = sqlpage.persist_uploaded_file('my_file', 'tests_uploads', 'txt', $mode);
select 'text' as component, $contents as contents;