Skip to content
Snippets Groups Projects
This project is mirrored from https://github.com/metabase/metabase. Pull mirroring updated .
  1. Jul 28, 2023
  2. Jan 27, 2023
  3. Oct 17, 2022
  4. Oct 10, 2022
  5. Oct 03, 2022
  6. Sep 13, 2022
    • Bryan Maass's avatar
      Hotfix metaboat165 for 43 (#25399) · e456b589
      Bryan Maass authored
      * H2 disallow `INIT=...` option using connection string (#25369)
      
      * trim the init option in h2 connection string
      
      - it can be lower or mixed-case, so `(dissoc-by str/lower-case ...)`
        will lowercase the map's keys and the dissoc-keys, and dissoc them when
        they are equal.
      
      * refactor
      
      * users may only send non-ddl native sql to h2 (#25220)
      
      * users may only send non-ddl native sql to h2
      
      * tests for diasllowing h2 ddl
      
      * improve error message on throw
      
      * fix linter + cleanup the-exploit example.
      
      * refix linter
      
      * handle garbage input by failing to classify it.
      
      If it h2 can't parse it, then that input cannot trigger a vulnerability.
      
      Our parser simply chews through erroneous sql, and classifies statements
      that it is able to parse.
      
      * When h2 is running in client-side mode, do not parse sql
      
      - SessionRemote indicates that we are currently client side
      
      * refactor so that building parser is easier
      
      * remove unused import SessionRemote
      
      * Revert "refactor so that building parser is easier"
      
      This reverts commit a41800131696de00d98e4eb7124d4d4b1b1cb33c.
      
      * check client-side conns => import SessionRemote
      
      * replace truncate function via inlining
      
      * fix drop arg order
      
      * linter fix
  7. Aug 11, 2022
  8. Aug 08, 2022
  9. Aug 03, 2022
  10. Jul 29, 2022
    • dpsutton's avatar
      Bump ssh to 2.9 for 43.x release (#24447) · 7809f829
      dpsutton authored
      
      * Bump ssh to 2.9 for 43.x release
      
      * Stop testing against Java 8 in CI
      
      Co-authored-by: default avatarCam Saul <github@camsaul.com>
    • dpsutton's avatar
      Ensure uploaded secrets are stable (#24325) (#24441) · 85055d28
      dpsutton authored
      * Ensure uploaded secrets are stable (#24325)
      
      * Ensure uploaded secrets are stable
      
      Fixes: https://github.com/metabase/metabase/issues/23034
      
      Background:
      Uploaded secrets are stored as bytes in our application db since cloud
      doesn't have a filesystem. To make db connections we stuff them into
      temporary files and use those files.
      
      We also are constantly watching for db detail changes so we can
      recompose the connection pool. Each time you call
      `db->pooled-connection-spec` we check if the hash of the connection spec
      has changed and recompose the pool if it has.
      
      Problem:
      These uploaded files have temporary files and we make new temp files
      each time we call `db->pooled-connection-spec`. So the hashes always
      appear different:
      
      ```clojure
      connection=> (= x y)
      true
      connection=> (take 2
                         (clojure.data/diff (connection-details->spec :postgres (:details x))
                                            (connection-details->spec :postgres (:details y))))
      ({:sslkey
        #object[java.io.File 0x141b0f09 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_1388256635324085910.tmp"],
        :sslrootcert
        #object[java.io.File 0x6f443fac "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_9248342447139746747.tmp"],
        :sslcert
        #object[java.io.File 0xbb13300 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_17076432929457451876.tmp"]}
       {:sslkey
        #object[java.io.File 0x6fbb3b7b "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_18336254363340056265.tmp"],
        :sslrootcert
        #object[java.io.File 0x6ba4c390 "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_11775804023700307206.tmp"],
        :sslcert
        #object[java.io.File 0x320184a0
        "/var/folders/1d/3ns5s1gs7xjgb09bh1yb6wpc0000gn/T/metabase-secret_10098480793225259237.tmp"]})
      ```
      
      And this is quite a problem: each time we get a db connection we are
      making a new file, putting the contents of the secret in it, and then
      considering the pool stale, recomposing it, starting our query. And if
      you are on a dashboard, each card will kill the pool of the previously
      running cards.
      
      This behavior does not happen with the local-file path because the
      secret is actually the filepath and we load that. So the file returned
      is always the same. It's only for the uploaded bits that we dump into a
      temp file (each time).
      
      Solution:
      Let's memoize the temp file created by the secret. We cannot use the
      secret as the key though because the secret can (always?) includes a
      byte array:
      
      ```clojure
      connection-test=> (hash {:x (.getBytes "hi")})
      1771366777
      connection-test=> (hash {:x (.getBytes "hi")})
      -709002180
      ```
      
      So we need to come up with a stable key. I'm using `value->string` here,
      falling back to `(gensym)` because `value->string` doesn't always return
      a value due to its cond.
      
      ```clojure
      (defn value->string
        "Returns the value of the given `secret` as a String.  `secret` can be a Secret model object, or a
        secret-map (i.e. return value from `db-details-prop->secret-map`)."
        {:added "0.42.0"}
        ^String [{:keys [value] :as _secret}]
        (cond (string? value)
              value
              (bytes? value)
              (String. ^bytes value StandardCharsets/UTF_8)))
      ```
      
      Why did this bug come up recently?
      [pull/21604](https://github.com/metabase/metabase/pull/21604) gives some
      light. That changed
      `(hash details)` -> `(hash (connection-details->spec driver details))`
      
      with the message
      
      > also made some tweaks so the SQL JDBC driver connection pool cache is
      > invalidated when the (unpooled) JDBC spec returned by
      > connection-details->spec changes, rather than when the details map
      > itself changes. This means that changes to other things outside of
      > connection details that affect the JDBC connection parameters, for
      > example the report-timezone or start-of-week Settings will now properly
      > result in the connection pool cache being flushed
      
      So we want to continue to hash the db spec but ensure that the spec is
      stable.
      
      * typehint the memoized var with ^java.io.File
      
      * Switch memoization key from String to vector of bytes
      
      Copying comment from Github:
      
      When you upload a sequence of bytes as a secret, we want to put them in
      a file once and only once and always reuse that temporary file. We will
      eventually hash the whole connection spec but I don't care about
      collisions there. It's only given the same sequence of bytes, you should
      always get back the exact same temporary file that has been created.
      
      So i'm making a function `f: Secret -> file` that given the same Secret
      always returns the exact same file. This was not the case before
      this. Each uploaded secret would return a new temporary file with the
      contents of the secret each time you got its value. So you would end up
      with 35 temporary files each with the same key in it.
      
      An easy way to get this guarantee is to memoize the function. But the
      secret itself isn't a good key to memoize against because it contains a
      byte array.
      
      If the memoization key is the byte-array itself, this will fail because
      arrays have reference identity:
      
      ```clojure
      user=> (= (.getBytes "hi") (.getBytes "hi"))
      false
      ```
      
      So each time we load the same secret from the database we get a new byte
      array, ask for its temp file and get a different temp file each time.
      
      This means that memoization cannot be driven off of the byte array. But
      one way to gain this back quickly is just stuff those bytes into a
      string, because strings compare on value not identity. This is what is
      currently in the PR (before this change). I was banking on the
      assumption that Strings are just opaque sequences of bytes that will
      compare byte by byte, regardless of whether those bytes make sense.
      
      But you've pointed out a good point that maybe that is flirting with
      undefined behavior. If we use the hash of the contents of the byte array
      as the memoization key with (j.u.Arrays/hashCode array), then we open
      ourselves up the (albeit rare) case that two distinct secret values hash
      to the same value. This sounds really bad. Two distinct secrets (think
      two ssh keys) but both would map to only one file containing a single
      ssh key.
      
      An easy way to have the value semantics we want for the memoization is
      just to call (vec array) on the byte array and use this sequence of
      bytes as the memoization key. Clojure vectors compare by value not
      reference. So two secrets would return the same file if and only if the
      sequence of bytes are identical, in which case we would expect the files
      to be identical. This gives me the same guarantee that I was wanting
      from the String behavior I used initially without entwining this with
      charsets, utf8, etc.
      
      * empty commit to trigger CI
    • Jeff Bruemmer's avatar
      docs - api hot fix backport (#24414) · f48665a0
      Jeff Bruemmer authored
      * update api doc code
      
      * update api docs
  11. Jul 28, 2022
  12. Jul 13, 2022
  13. Jul 12, 2022
  14. Jul 08, 2022
  15. Jul 07, 2022
  16. Jul 04, 2022
  17. Jun 30, 2022
  18. Jun 29, 2022
  19. Jun 27, 2022
  20. Jun 26, 2022
    • Nemanja Glumac's avatar
      :cherries: Manual backport of E2E/Percy related changes (#23552) · d3f0596a
      Nemanja Glumac authored
      
      * Run Percy inside E2E main workflow (#23480)
      
      * Add Percy to the main E2E workflow
      
      * Ignore unit tests
      
      * Delete Percy workflow
      
      * Do not run Percy tests on Cypress changes only
      
      * Check whether visual regression tests should run at all
      
      * Remove diff logic from Percy job on `master` (#23551)
      
      * Ignore all unit tests in E2E workflows (#23550)
      
      * [E2E] Remove filters and onboarding checks from CCI (#23517)
      
      Co-authored-by: default avatarDiogo Mendes <diogo@metabase.com>
    • dpsutton's avatar
      Join slack channels with slack-id (#23495) (#23542) · 547964aa
      dpsutton authored
      * Join slack channels with slack-id
      
      Fixes https://github.com/metabase/metabase/issues/23229
      
      We upload images to a channel and then send messages to the desired
      channel referencing those images from the channel we uploaded. But the
      slack bot must be in the channel to upload images.
      
      We handle this in `slack/upload-image!` where we watch the error message
      and join the channel if we recognize that is our issue. This special
      upload channel is set in the admin section when setting up, typed in by
      a human.
      
      Slack now requires us to use the internal id of the channel when
      joining. IE we used to use "metabase_files" but now we need to use
      something like "C87LQNL0Y23". But we do not have this information and we
      don't want humans to have to look this up.
      
      SOLUTION:
      change our cache. We currently just get a list of channels and users
      ```
      ["#general" "@dan" ...]
      ```
      
      Change this to
      ```
      {:version 2,
       :channels [{:display-name "#random",
                   :name "random",
                   :id "CT2FNGZSRPL",
                   :type "channel"}
                  {:display-name "#general",
                   :name "general",
                   :id "C87LQNL0Y23",
                   :type "channel"}
                  {:display-name "@dan",
                   :type "user",
                   :name "dan",
                   :id "UR65C4ZJVIW"}
                  ...]}
      ```
      
      Now we have slack internal ids present. When we attempt to join the
      slack channel, look for this id and attempt to use that.
      
      This has some knock-on effects. The UI still lists the channels in a
      channel picker when sending pulses. The list sent over the wire still
      mimics the previous shape (a flat list) and the choice is still the
      human readable name.
      
      In the future we should switch over to using the stable ids rather than
      solely channel names. Channel names can be renamed.
      
      I didn't go down this route because of the files channel. It is set at
      setup before we have a channel list. We could do some kind of run time
      migration but it is difficult because it would change the type of
      `slack-files-channel` from string to :json to handle the more complex
      type. Or perhaps we could make another setting to hold the json form and
      set that when we can positively identify things.
      
      In either case, these changes were not required yet to fix our slack
      issue. We just upgrade the information we have about slack channels,
      downgrade it when it hits the wire so the UI needs no changes, and use
      the extra information in the one spot where we need it.
      
      The cache is populated at startup and every four hours after that. So we
      do not need to worry about the old cache shape. If the new code is
      running, its the new cache.
      
      * Send #channel and @user forms over wire
      
      We store `{"channel": "#slack-pulses"}` in the pulse_channel.details
      column so we should keep those types of values around.
      
      We use the bare portion ("slack-pulses") rather than with the hash on it
      so we seem to be mixing usernames and channels. But these sets are
      distinct and you cannot create a channel with the same name as a
      user. Also, channel names are lowercase while channel-ids are uppercase
      so those are also non-overlapping sets.
      
      * Put slack token so slack reports as configured
      
      * Errant tap>
      
      * Alignment and docstring fixes
      
      * Remove slack-cache version information
      
      remove the `:version 2` from the cache. We are always in charge of the
      cache, and we compute it on startup so there's little risk of other data
      shapes being present.
    • github-actions[bot]'s avatar
  21. Jun 24, 2022
Loading