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

fix(fs): Escape paths in Scope methods #2070

Merged
merged 9 commits into from
Nov 21, 2024
Merged

fix(fs): Escape paths in Scope methods #2070

merged 9 commits into from
Nov 21, 2024

Conversation

FabianLars
Copy link
Member

opening as a draft because it doesn't compile locally, complaining about glob not being a dependency but only if it's uses in scope.rs - if it fails in ci too i'll just move it to lib.rs.

if anyone spots my mistake here, please enlighten me...

fixes tauri-apps/tauri#11707
fixes tauri-apps/tauri#11708

Copy link
Contributor

github-actions bot commented Nov 18, 2024

Package Changes Through cc182fb

There are 15 changes which include upload with minor, upload-js with minor, deep-link with patch, deep-link-js with patch, fs with minor, persisted-scope with minor, log-plugin with patch, log-js with patch, fs-js with patch, localhost with minor, opener with major, opener-js with major, positioner-js with minor, positioner with minor, sql with patch

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
api-example 2.0.5 2.0.6
api-example-js 2.0.2 2.0.3
deep-link-example-js 2.0.0 2.0.1
deep-link 2.0.1 2.0.2
deep-link-js 2.0.0 2.0.1
fs 2.0.3 2.1.0
fs-js 2.0.2 2.0.3
dialog 2.0.3 2.0.4
opener 1.0.0 2.0.0
opener-js 1.0.0 2.0.0
http 2.0.3 2.0.4
localhost 2.0.1 2.1.0
log-plugin 2.0.2 2.0.3
log-js 2.0.0 2.0.1
persisted-scope 2.0.3 2.1.0
positioner 2.0.2 2.1.0
positioner-js 2.0.1 2.1.0
single-instance 2.0.1 2.0.2
sql 2.0.2 2.0.3
upload 2.1.0 2.2.0
upload-js 2.1.0 2.2.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@amrbashir
Copy link
Member

amrbashir commented Nov 18, 2024

this is because the scope module is used in build.rs, see

#[path = "src/scope.rs"]
#[allow(dead_code)]
mod scope;

you can just add glob to the plugin build-dependencies and it should work

@FabianLars
Copy link
Member Author

Ahhhhhh yes, forgot about that...

@FabianLars
Copy link
Member Author

Oh and thanks. I'll fix it when I'm back

@FabianLars FabianLars marked this pull request as ready for review November 18, 2024 22:24
@FabianLars FabianLars requested a review from a team as a code owner November 18, 2024 22:24
@FabianLars
Copy link
Member Author

this seems to work for now but i'm less than happy.

i think it may make sense to release this is a temp fix/workaround and wait for lucas to get back to discuss further steps.

Alternatively i'm thinking about storing a tauri::scope::fs::Scope inside of tauri_plugin_fs::Scope and using that to keep track of the actual scope instead of the PathBuf Vec (keeping the latter around to not have a breaking change...). But ideally really we could deduplicate the scope definitions themselves but i'm sure that was already tried.

idk, i guess i'll take another look tomorrow.

plugins/fs/Cargo.toml Outdated Show resolved Hide resolved
plugins/fs/src/scope.rs Outdated Show resolved Hide resolved
plugins/fs/src/scope.rs Outdated Show resolved Hide resolved
plugins/fs/src/scope.rs Outdated Show resolved Hide resolved
@amrbashir
Copy link
Member

Alternatively i'm thinking about storing a tauri::scope::fs::Scope inside of tauri_plugin_fs::Scope and using that to keep track of the actual scope instead of the PathBuf Vec (keeping the latter around to not have a breaking change...). But ideally really we could deduplicate the scope definitions themselves but i'm sure that was already tried.

actually I think we maybe able to reuse the one from tauri::scope::fs::Scope and looking at the fs plugin, it doesn't actually make sense to have a separate implementation at all, but let's refactor that in another PR.

@FabianLars FabianLars marked this pull request as draft November 18, 2024 23:23
@FabianLars
Copy link
Member Author

i already miss the time when it was ok to break user facing apis 😂

what do you think about the new approach?

@FabianLars
Copy link
Member Author

also thought about a new FsExt where fs_scope() returns a new scope based on tauri's fs::Scope and maybe making the old apis no-op but idk, everything i can think of is ugly

Ok(path)
} else {
Err(CommandError::Plugin(Error::PathForbidden(path)))
}
}

fn is_forbidden<P: AsRef<Path>>(
Copy link
Member

Choose a reason for hiding this comment

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

why is this implemented separately here?

Copy link
Member Author

@FabianLars FabianLars Nov 19, 2024

Choose a reason for hiding this comment

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

i can implement it in tauri::fs::Scope too and re-use that but wanted to keep the plugin compatible with all tauri v2 versions for now.

not too happy about the approach itself but merging instances of tauri::fs::Scopes isn't possible rn so this was the next best thing to make the draft work.

Copy link
Member

Choose a reason for hiding this comment

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

let's ship with this for now, but I think we should still add it in tauri and then migrate the fs plugin later to that.

Copy link
Member Author

@FabianLars FabianLars Nov 21, 2024

Choose a reason for hiding this comment

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

ok, will open a pr later if i don't forget

plugins/fs/src/scope.rs Outdated Show resolved Hide resolved
@FabianLars FabianLars marked this pull request as ready for review November 20, 2024 19:57
@FabianLars
Copy link
Member Author

this should be done now apart from the (ongoing?) discussion about is_forbidden

plugins/persisted-scope/src/lib.rs Outdated Show resolved Hide resolved
plugins/dialog/src/commands.rs Outdated Show resolved Hide resolved
Ok(path)
} else {
Err(CommandError::Plugin(Error::PathForbidden(path)))
}
}

fn is_forbidden<P: AsRef<Path>>(
Copy link
Member

Choose a reason for hiding this comment

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

let's ship with this for now, but I think we should still add it in tauri and then migrate the fs plugin later to that.

Comment on lines +1027 to +1033
if is_forbidden(&fs_scope.scope, &path, require_literal_leading_dot)
|| is_forbidden(&scope, &path, require_literal_leading_dot)
{
return Err(CommandError::Plugin(Error::PathForbidden(path)));
}

if fs_scope.scope.is_allowed(&path) || scope.is_allowed(&path) {
Copy link
Member

Choose a reason for hiding this comment

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

do really need to manually call is_forbidden, doesn't scope.is_allowed check for forbidden/denied patterns?

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem is that is_allowed returns false for paths that are explictly denied and paths that are neither allowed nor denied.
When we're checking for 2 scopes at the same time we only care about the explicitly denied paths because those take precedence of explictly allowed paths.
A path that's neither allowed nor denied in the first scope could still be explicitly allowed by the second scope.

Alternatively a way to create a new Scope from 2 existing scopes would be cool too, then is_allowed would work for us here.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can improve this a bit by adding the Scope::is_forbidden in tauri

amrbashir
amrbashir previously approved these changes Nov 21, 2024
@amrbashir amrbashir merged commit fecfd55 into v2 Nov 21, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants