Add a whitelist for info keys on var metadata

[hugoduncan]

When generating a map for info on a var, whitelist the keys that may appear.

Address cider #588

[bbatsov]

The list seems incomplete. See the comment made by @jeffvalk here #65

[hugoduncan]

@bbatsov do all those apply to clojure vars?

[cichli]

cljs-tooling also emits :author for namespace info - worth including? I'm not sure if it's actually used by CIDER or not.

[hugoduncan]

@cichli This specific whitelist is only applied to clojure vars, iiuc.

[cichli]

You are, of course, correct. info-cljs selects only the keys :file :line :ns :doc :column :name :arglists.

[bbatsov]

@hugoduncan The build is failing.

@jeffvalk @gtrak What do you think? Personally I don't like having both this and the existing blacklist; seems a bit confusing this way.

[jeffvalk]

Perhaps I haven't followed this issue closely enough... I understand that anything hypothetically could be populated with an unsupported value type, but which metadata attributes have we actually seen cause an issue other than the blacklisted :arglists and :forms?

[hugoduncan]

@jeffvalk We use :sigs metadata on functions to carry schema based function signatures. We also have defmulti like vars that carry their dispatch maps in metadata.

[hugoduncan]

@bbatsov the existing blacklist seemed to be a bit of a sledgehammer, being applied to metadata whatever the source. I felt that having a black/whitelist per metadata source makes more sense - clojurescript functions already have an implicit whitelist in the code.

[jeffvalk]

@hugoduncan Understood. That definitely adds context.

@bbatsov Agreed --if we're using a whitelist, the blacklist isn't helpful. And a whitelist still doesn't guarantee its attributes' data types (I think @gtrak pointed this out on a related thread) -- If someone sticks unserializable data in :doc, things still break.

[jeffvalk]

I'm still playing catch up on this, so I may be oversimplifying... My understanding is the core of this issue is that bencode dictionary keys must be strings. If so, what cases would just stringifying the key not let us accommodate?

[jeffvalk]

I tried the patch in the above gist with the :sigs metadata from CIDER #588 and it looks fine. What am I missing?

[bbatsov]

My understanding is the core of this issue is that bencode dictionary keys must be strings.

Pretty much - yes. I'm not sure if you've read the discussion in #61. There stringifying the keys was also mentioned.

[jeffvalk]

Ah, ok. Hadn't read that in detail. Is there a drawback to that approach? It should be safer than either a blacklist or whitelist.

[bbatsov]

Not sure. Haven't given that much thought after the whitelist idea was originally presented a while back. After all, as we know what data is required by cider, it seems wasteful to bloat the responses unnecessary (especially given the fact we have to occasionally read them :-) ).

As it stands we're quite certain about the structure of all required attributes (unless I'm missing something), except the arglists which we already handle specially, anyways.

[jeffvalk]

Completely agree that limiting metadata returned by info to what's expected/used is preferable. My thinking here is just that since we know non-string keys aren't ok to send, we should make sure they can't happen.

[bbatsov]

Sure. But this is only a problem with collection types (maps or collections with maps in them). If some metadata value is not a collection there's no chance anything in it will be encoded as a dictionary.

[jeffvalk]

Right, but the whitelist alone doesn't guarantee the data type of those metadata properties. If I use one of the whitelisted keys to point to some arbitrarily nested collection (say I think :added has an application-specific use), the unserializable data is let past the whitelist.

[bbatsov]

You're correct, but consider this as well - if the data that's fed to CIDER doesn't have the format it expects there'll be a client-side error anyways. In other terms, CIDER might expect the contents of a string in a response to be some number, but they might be something else - I don't think there's a sane way to handle that.

I understand our problem, but I can't think of a real solution. Converting to strings would suppress the decode errors, but will still leave us vulnerable to problems.

[jeffvalk]

That's certainly true. But decoding errors are quite opaque, and I think we should be keen to eliminate them wherever possible. An error at a higher level is still an error, but it's likely to be more specific and easier to diagnose. wrong type argument: stringp, (non-string data that I recognize) coming from, say, the doc function paints a clearer picture of the actual problem than the sort of error we'd get from nrepl-decode.

IMO, calling str on map keys when the spec requires that all keys be strings is sensible. At the very least it clarifies intent. It can't harm anything (if it does, that's a bug), and it prevents one rather ugly class of errors...even if others do remain possible.

[bbatsov]

@jeffvalk OK, I'm sold. :-) Please, go ahead and make the necessary changes.

[bbatsov]

@hugoduncan Thanks! 👍