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.

Sign up for GitHub

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 deno.open permission check #1336

Merged
merged 1 commit into from
Dec 13, 2018
Merged

Fix deno.open permission check #1336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 11 additions & 1 deletion js/deno.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,17 @@
/// <amd-module name="deno"/>
export { env, exit } from "./os";
export { chdir, cwd } from "./dir";
export { File, open, stdin, stdout, stderr, read, write, close } from "./files";
export {
File,
open,
stdin,
stdout,
stderr,
read,
write,
close,
OpenMode
} from "./files";
export {
copy,
toAsyncIterator,
Expand Down
2 changes: 1 addition & 1 deletion js/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const stdout = new File(1);
/** An instance of `File` for stderr. */
export const stderr = new File(2);

type OpenMode =
export type OpenMode =
/** Read-only. Default. Starts at beginning of file. */
| "r"
/** Read-write. Start at beginning of file. */
Expand Down
20 changes: 18 additions & 2 deletions js/files_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,26 @@ test(async function filesToAsyncIterator() {
assertEqual(totalSize, 12);
});

testPerm({ write: false }, async function writePermFailure() {
const filename = "tests/hello.txt";
const writeModes: deno.OpenMode[] = ["r+", "w", "w+", "a", "a+", "x", "x+"];
for (const mode of writeModes) {
let err;
try {
await deno.open(filename, mode);
} catch (e) {
err = e;
}
assert(!!err);
assertEqual(err.kind, deno.ErrorKind.PermissionDenied);
assertEqual(err.name, "PermissionDenied");
}
});

testPerm({ write: true }, async function createFile() {
const tempDir = await deno.makeTempDir();
const filename = tempDir + "/test.txt";
let f = await deno.open(filename, "w");
const f = await deno.open(filename, "w");
let fileInfo = deno.statSync(filename);
assert(fileInfo.isFile());
assert(fileInfo.len === 0);
Expand Down Expand Up @@ -88,7 +104,7 @@ testPerm({ write: true }, async function openModeWriteRead() {
const filename = tempDir + "hello.txt";
const data = encoder.encode("Hello world!\n");

let file = await deno.open(filename, "w+");
const file = await deno.open(filename, "w+");
// assert file was created
let fileInfo = deno.statSync(filename);
assert(fileInfo.isFile());
Expand Down
27 changes: 24 additions & 3 deletions src/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,15 +576,15 @@ fn op_chmod(
}

fn op_open(
_state: &IsolateState,
state: &IsolateState,
base: &msg::Base,
data: libdeno::deno_buf,
) -> Box<Op> {
assert_eq!(data.len(), 0);
let cmd_id = base.cmd_id();
let inner = base.inner_as_open().unwrap();
let filename = PathBuf::from(inner.filename().unwrap());
// let perm = inner.perm();
let filename_str = inner.filename().unwrap();
let filename = PathBuf::from(&filename_str);
let mode = inner.mode().unwrap();

let mut open_options = tokio::fs::OpenOptions::new();
Expand All @@ -594,28 +594,49 @@ fn op_open(
open_options.read(true);
}
"r+" => {
if let Err(e) = state.check_write(&filename_str) {
Copy link
Contributor

@F001 F001 Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

These checks are repeative. What about combining them together?

// add this before the `match mode`
match mode {
"r+" | "w" | "w+" | "a" | "a+" | "x" | "x+" => {
    if let Err(e) = state.check_write(&filename_str) {
        return odd_future(e);
      }
 }
 _ => {}
}
// original code
match mode {
// set open_options
}

Copy link
Contributor Author

@kevinkassimo kevinkassimo Dec 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, does this slightly add some overhead by doing 2 separate matchings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the r mode the only one that does not need write permission? What about

// original code
match mode {
}
if mode != "r" { // mode is guaranteed to be valid
  // only need to write check_write() once
}

return odd_future(e);
}
open_options.read(true).write(true);
}
"w" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options.create(true).write(true).truncate(true);
}
"w+" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options
.read(true)
.create(true)
.write(true)
.truncate(true);
}
"a" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options.create(true).append(true);
}
"a+" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options.read(true).create(true).append(true);
}
"x" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options.create_new(true).write(true);
}
"x+" => {
if let Err(e) = state.check_write(&filename_str) {
return odd_future(e);
}
open_options.create_new(true).read(true).write(true);
}
&_ => {
Expand Down