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

Unable to import json files #5633

Closed
frunkad opened this issue May 19, 2020 · 19 comments
Closed

Unable to import json files #5633

frunkad opened this issue May 19, 2020 · 19 comments
Labels
cli related to cli/ dir stale

Comments

@frunkad
Copy link

frunkad commented May 19, 2020

While trying to import any json file, the following error is printed:

error: Uncaught TypeError: Cannot resolve extension for "file:///home/frunkad/.../lastRelease.json" with mediaType "Json".
    at getExtension ($deno$/compiler.ts:218:13)
    at new SourceFile ($deno$/compiler.ts:263:22)
    at Function.addToCache ($deno$/compiler.ts:339:16)
    at processImports ($deno$/compiler.ts:743:31)
    at async processImports ($deno$/compiler.ts:753:7)
    at async compile ($deno$/compiler.ts:1316:31)
    at async tsCompilerOnMessage ($deno$/compiler.ts:1548:22)
    at async workerMessageRecvCallback ($deno$/runtime_worker.ts:74:9)

Deno Version

deno 1.0.0
v8 8.4.300
typescript 3.9.2

Steps to Reproduce

  1. Install Deno
  2. Update .bashrc or .bash_profile
  3. Run the following with deno run index.ts
import data from './lastRelease.json';
console.log(data);

As per #1048 also tried import * as data from './lastRelease.json' but with the same error.

At first, I thought it to be an issue with my machine but tried running the same on a cloud server and it gave the same result.

@sholladay
Copy link

Unfortunately, they removed support for importing JSON shortly before the final 1.0 release shipped. See: #5037

Apparently there was some security issue with it. The nature of the issue is unclear to me.

@Soremwar
Copy link
Contributor

Soremwar commented May 19, 2020

JSON imports were also rolled back in browser because of the security implications they represented.

Deno code is compiled without read access requested to the user(intended behavior, this is harmless), but a JSON file could effectively contain sensitive data from the user. The import/export system can access files from anywhere in the computer, so join these two together and you got a security breach.

EDIT: A patch was implemented to request read permissions when reading JSON data, however the solution didn't seem to be the liking of Deno devs.

@nayeemrmn
Copy link
Collaborator

@sholladay For Deno the security wasn't much of an issue because we could just apply read permission to JSON imports. The issue is that browsers don't support it.

@sholladay
Copy link

sholladay commented May 19, 2020

Browsers do not provide access to the filesystem, so why did they roll it back?

Also, last time I tried to use it, dynamic import() in Deno does require permissions. So we should be allowed to import JSON through it. Doesn't really make sense to disallow it there, because what the community will end up doing as a workaround is to read the filesystem and parse the file manually with JSON.parse(). People can already import JSON this way, it's just less convenient.

If we can agree that JSON imports are acceptable so long as permissions are requested, then to me the question arises, "Why not support permissioned static imports of JSON?"

@matthieudesprez
Copy link

matthieudesprez commented May 19, 2020

You can use Deno.readTextFileSync along with JSON.parse

const data = JSON.parse(Deno.readTextFileSync('./filename.json'));

IMHO it makes more sense to reserve import for modules

@kitsonk
Copy link
Contributor

kitsonk commented May 19, 2020

@sholladay the nature of the security issue is discussed here: #3401 (comment)

Relying just on the media type to parse JSON leads to security attack vectors that would be hard to close, which is why browsers are disallowing JSON imports. We were relying on the media type, and would have to put in a lot of other checks, of which if browsers have decided to disallow it, it is best we don't try to fight against that. Just having access to the file system isn't a requirement to create a security exploit.

@sholladay
Copy link

sholladay commented May 20, 2020

I agree that permissionless access to data files (of any type, not just JSON) is potentially problematic. Is that what you're referring to?

Dynamic import() of JSON could just be sugar for Deno.readTextFile('...').then(JSON.parse), which requires permissions. Is there a security problem with doing that? Because the community will absolutely do so, in a bespoke fashion, if it's not built in. For context, Sindre's load-json-file npm module is downloaded more than 23 million times per week! Granted, some of that is tied to the prevalence of package.json files in the Node ecosystem, but still, this is a very common use case.

@kitsonk
Copy link
Contributor

kitsonk commented May 20, 2020

Dynamic import() of JSON could just be sugar for Deno.readTextFile('...').then(JSON.parse).

Not exactly... Code loaded through import() is always injected into the v8 isolate via the module loading paths, which instantiates objects (and has its own context) and populates the in memory module cache in v8. There was quite complex logic to determine a shape of a "JSON module" in Rust before loading it into the isolate. Quite a bit different to a JSON.parse.

@frunkad
Copy link
Author

frunkad commented May 20, 2020

Because the community will absolutely do so, in a bespoke fashion, if it's not built in. For context, Sindre's load-json-file npm module is downloaded more than 23 million times per week!

Whenever I have imported JSON files - they were either configuration files or large data files. I'll agree, the first workaround that I thought of was using file system. But the workaround I used was changing the config file to config.ts.

It took 0 effort to do so no need to allow file system access.

For larger files, in all my use cases they were required asynchronously and very few times in any project. Having to use readTextFile won't be an issue, personally, and I believe for the community at large.

@bartlomieju bartlomieju added the cli related to cli/ dir label May 26, 2020
@jsejcksn
Copy link
Contributor

jsejcksn commented Jun 7, 2020

@frunkad See readJson and readJsonSync at https://doc.deno.land/https/deno.land/std/fs/mod.ts#readJson

import {readJson} from 'https://deno.land/std/fs/mod.ts';

const data = await readJson('/path/to/data.json');

@kitsonk It seems odd that there is a discrepancy between the type that is returned form these functions (unknown) and JSON.parse (any). Can you point me to a discussion about the reasoning for this?

@mfulton26
Copy link

@kitsonk It seems odd that there is a discrepancy between the type that is returned form these functions (unknown) and JSON.parse (any). Can you point me to a discussion about the reasoning for this?

#6438

@kitsonk
Copy link
Contributor

kitsonk commented Jul 10, 2020

There has been recent progress on this within TC39. The proposal for syntax to support loading JSON ES Modules has been proposed: https://github.com/tc39/proposal-import-conditions. It is Stage 2 currently, but we should keep a close eye on it.

@sno2
Copy link
Contributor

sno2 commented Sep 15, 2020

@frunkad See readJson and readJsonSync at https://doc.deno.land/https/deno.land/std/fs/mod.ts#readJson

import {readJson} from 'https://deno.land/std/fs/mod.ts';

const data = await readJson('/path/to/data.json');

@kitsonk It seems odd that there is a discrepancy between the type that is returned form these functions (unknown) and JSON.parse (any). Can you point me to a discussion about the reasoning for this?

readJson & readJsonSync are now gone btw

@jsejcksn
Copy link
Contributor

readJson & readJsonSync are now gone btw

Yes, removed in #7255 and #7256.

When they were created, the Deno.writeTextFile API didn't exist yet, but now you can re-implement them trivially with:

// write
await Deno.writeTextFile('path/to/file.json', JSON.stringify(data));
// write sync
Deno.writeTextFileSync('path/to/file.json', JSON.stringify(data));

// read
JSON.parse(await Deno.readTextFile('path/to/file.json'));
// read sync
JSON.parse(Deno.readTextFileSync('path/to/file.json'));

If you want to see the options available in the functions, they can be viewed at the last available versions in [email protected] (read, write).

@jerrygreen
Copy link

jerrygreen commented Sep 27, 2020

If I use local paths like:

JSON.parse(await Deno.readTextFile('path/to/file.json'));

Then it's ok (probably). But if I want to get some remote json like so (for example):

const json = await fetch("https://path.to/file.json").then((x) => x.json());

Then caching mechanisms of Deno won't be applied, and each time I run my Deno script, the file will be downloaded. So -r flag is useless in this case, since cache isn't working anyway...

I vote for bringing json static imports back! But not permissionless ofc: with support of --allow-net for remote addresses, and with --allow-read for local addresses, and with caching. Where to sign a petition?

@jsejcksn
Copy link
Contributor

@jerrygreen

JSON imports: See #7623.

Caching: The Deno cache is only for modules, but there is discussion about storage in #1657, and there are existing libraries to help you implement cache management of remote resources. A few examples: deno-sqlite, deno_sqlite_plugin, deno-redis

@jerrygreen
Copy link

@jsejcksn, naah, inventing some own cache management on application level just to import json, - it's a bit overkill in most times. I personally wanted it to quickly hack few things together for personal needs, not to make some production-ready solid business app. I thought that there's already cache for module imports, - they aren't downloaded on each run, so it's very natural to have something like this maybe:

import json from "https://jsonplaceholder.typicode.com/todos/1" as "json"

^ When Import Assertions will come to live, at least.

or this:

import json from "https://jsonplaceholder.typicode.com/todos/1.json"

^ I personally think it should work too, and in most cases should be sufficient, and it was working in some early versions of Deno, it seems. And, actually, I expected it to work... But it turned out json imports are not a thing anymore :(

My only concerns here:

  1. json imports is a good thing (please implement 😢)
  2. the same caching logic used for module imports, - it should be applied to json imports too (not only for consistency, but also for speed 💪 and for simplicity of application-level code)

Just a little bit of a feedback. Sorry for whining, and thanks for listening.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 6, 2021
@kitsonk
Copy link
Contributor

kitsonk commented Jan 7, 2021

It is currently working as designed and this will be addressed under #7623.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir stale
Projects
None yet
Development

No branches or pull requests